-
Notifications
You must be signed in to change notification settings - Fork 1
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
best::tap::operator[]
index mismatch
#16
Comments
As an aside, replacing
I don't understand why this lint is unable to notice the bug when |
Interesting. That is indeed a problem. I think this is enough of a footgun that we should either remove The reason |
There are two issues with
best::tap::operator[]
-- a typo that prevents use, and a lifetime issue.Here is the current implementation, for reference.
best/best/func/tap.h
Lines 178 to 201 in 3475255
First
There is a name collision for
arg
-- the lambda argument shadows the function argument. This prevents any actual use. The solution is straightforward: change one of the names, add a few tests to prevent any future issues.Second
We run into some weird lifetime issues here. In general,
operator[]
returns reference types. Thedecltype(auto)
signature preserves the reference. This means that code like this:Will return a dangling reference to the objects on the stack of the lambda we just left:
There are scenarios where this behavior is valid. If the
tap
produces, say, a reference to a static object, than references into that container are presumably also static, and valid. However, I believe the average case is a stack-scoped return from.The documentation alludes to oddities with this behavior, but stops short of fully describing the behavior:
As far as resolution, I am happy to submit a PR, and have two proposed resolutions:
Option 1: Leave it alone
In short, just fix the argument naming, and add a sentence in the documentation for
best::tap
explaining the behavior outlined above. This maintains maximal flexibility for the user, even if it is a bit of a foot-gun.Option 2: Prefer return-by-value
We can also just update the return types of the function to return a
best::unref<decltype(/* the entire function body */)>
. Returning-by-value, I believe, will present a more ergonomic behavior for library users. It does, however, introduce 'hidden' costs(an implicit copy constructor call), and prevent more advanced users for intentionally returning references.I will submit a PR for whichever option you prefer, @mcy.
The text was updated successfully, but these errors were encountered: