Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reset to CowData, to bring it on-par with other collections. #101393

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Contributor

In most APIs, clear() destroys all elements but keeps the capacity intact. This is the case for STL collections, as well as LocalVector. LocalVector additionally declares reset to give up the backing buffer.

CowData and related classes have no way to shrink without giving up the buffer (godotengine/godot-proposals#2954). As such, clear() behavior cannot be changed to be on-par with LocalVector. But we can certainly bring the APIs closer together by adding reset() and letting users use that instead of clear() if an explicit dealloc of the buffer is wanted (which is probably most cases).

…Add comments to `clear` to warn about the discrepancy between this API and others.
@Ivorforce Ivorforce requested a review from a team as a code owner January 10, 2025 14:56
@AThousandShips
Copy link
Member

AThousandShips commented Jan 10, 2025

Not sure about this one, it's identical to clear and is only present in three other types, LocalVector, AHashMap and HashMap, and only the first is really relevant for that, so I don't think there's much syncing up with other containers here

I don't see the usefulness of this, which should be the guiding principle behind new features

@Ivorforce
Copy link
Contributor Author

Not sure about this one, it's identical to clear and is only present in two other types, LocalVector and HashMap, and only the former is really relevant for that, so I don't think there's much syncing up with other containers here

I don't see the usefulness of this, which should be the guiding principle behind new features

For me this is about API consistency, which is key to reducing complexity in large codebases. imo, it was incorrect to call the function clear in the first place. Ideally, this would be amended in the future.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 10, 2025

But this doesn't add consistency, it adds something that's present in three out of 15 or more classes to three more, that's reducing consistency

If it did something unique I'd say it would make sense, but now it just creates an alias for clear which is just confusing

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Jan 10, 2025

But this doesn't add consistency, it adds something that's present in three out of 15 or more classes to three more, that's reducing consistency

If it did something unique I'd say it would make sense, but now it just creates an alias for clear which is just confusing

😄 you got a point there!
I did expect more containers to offer reset so I'm somewhat surprised this is not the case.

The reason I chose reset for this function is that, whenever collections have both, clear means "destruct objects but not buffer", but reset means "also destruct buffer". This is also consistent with STL's concept of clear.
In that sense, I'd view renaming clear to reset in all other cases as the best choice. But that's very large task, and the similar discussion about ptr() isn't even through yet (#100555), so I wanted to see if people generally agree we should attempt to work towards consistency in this way.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 10, 2025

I'd say consistency in this case means "a class has the function if it actually needs it", not "slap the function on anything".

Also this method should either be bound on String or not added to String, otherwise it adds an annoying additional thing to keep an eye out for when dealing with extensions and modules etc. because String is a bound class (it would have to be added to godot-cpp as well) and should also be added to Packed*Array binds for the same reason

The exact same concept applies to ptr/ptrw IMO

@Ivorforce
Copy link
Contributor Author

Also this method should either be bound on String or not added to String

Yeah, that's probably true! Valid reason to stall merging this PR until clear can actually be implemented differently (and even then we'll have future API compatibility concerns...).

@AThousandShips
Copy link
Member

Agreed, I think adding a reset before it actually is functionally different will either:

  • Never be used as it doesn't make any difference, or,
  • Will be used inconsistently and will make changing it to actually have distinct functionality a mess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants