-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fix ForeverStack::find_starting_point
output parameter
#3247
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against this, but what is the issue with the current way of doing it?
@@ -493,12 +494,12 @@ ForeverStack<N>::resolve_path (const std::vector<S> &segments) | |||
if (segments.size () == 1) | |||
return get (segments.back ().as_string ()); | |||
|
|||
auto starting_point = cursor (); | |||
std::reference_wrapper<Node> starting_point = cursor (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we can't use auto
anymore for getting the cursor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, starting_point
was of type Node
and not Node &
-- so find_starting_point
and resolve_path
were copying nodes instead of adjusting a reference to a node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I made this patch, I didn't notice that it was auto starting_point
instead of auto &starting_point
, so I thought that in some cases the code would clobber parts of the ForeverStack
-- but it would probably be best to avoid the Node
copies anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To sum it up for clarity, find_starting_point
should probably take a reference to a reference to a Node
, rather than a reference to a Node
gcc/rust/ChangeLog: * resolve/rust-forever-stack.h (ForeverStack::find_starting_point): Use type 'std::reference_wrapper<Node> &' instead of 'Node &' for parameter starting_point. * resolve/rust-forever-stack.hxx (ForeverStack::find_starting_point): Likewise. (ForeverStack::resolve_path): Handle change to ForeverStack::find_starting_point. Signed-off-by: Owen Avery <[email protected]>
@CohenArthur @philberty thoughts on merging? |
No description provided.