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

nit: require a HasUnit trait? #5870

Open
bennetyee opened this issue Sep 27, 2024 · 0 comments
Open

nit: require a HasUnit trait? #5870

bennetyee opened this issue Sep 27, 2024 · 0 comments

Comments

@bennetyee
Copy link
Contributor

pub fn get_product(&self, index: usize) -> Option<T> {

users of this tend to have to write foo.get_product().unwrap_or(XXX) where XXX is the ONE value for T.

maybe it would be better to require T to implement something like a HasUnit trait that provides a T::ONE value as a constant, to be returned instead of the None? i don't know if the None return with the unwrap_or is at all guaranteed to be optimized out (are there separate compilation issues?) but even ignoring that I would think not having to do the unwrap_or at every call site would make the using code (slightly) more readable.

is there a case to be made for detecting when the tree of products is empty and handling that differently via the None return to distinguish the case where a non-empty tree might return the 1 value?

currently this has undocumented semantics wrt out of range index values yielding the product of all values, though maybe arguably this is covered by 'except the one at the given index' because there aren't any values at the index? the test verifies this behavior -- and i think we probably should just test use the size (for InternalNode) to panic when out of range.

@bennetyee bennetyee changed the title require a HasUnit trait? nit: require a HasUnit trait? Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant