-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 3845 nullptr cpp #3958
Fix 3845 nullptr cpp #3958
Conversation
…ay of parent contexts, last one of which is nullptr. Signed-off-by: Terence Parr <parrt@antlr.org>
Signed-off-by: Terence Parr <parrt@antlr.org>
if (lhs == nullptr && rhs == nullptr) return true; | ||
if (lhs != nullptr || rhs != nullptr) return false; | ||
return *lhs == *rhs; |
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.
It looks incorrect. Last line is never executed since it return false
if any or both arguments are not null. Also, it's not optimal. I suggest fixing in the following way:
if (lhs == nullptr && rhs == nullptr) return true; | |
if (lhs != nullptr || rhs != nullptr) return false; | |
return *lhs == *rhs; | |
if (lhs == nullptr) | |
return rhs == nullptr; | |
else | |
return rhs != nullptr; | |
return *lhs == *rhs; |
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.
dang! you're right. I tried to get too clever. grrr...will fix. thanks.
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.
is your line 32 dead code?
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.
maybe this?
if ( lhs == nullptr ) return rhs == nullptr;
if ( rhs == nullptr ) return false; // lhs!=null and rhs==null
return *lhs == *rhs; // both nonnull
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.
Not dead, it's comparison by dereference (if I remember and understand C++ correctly).
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.
meaning your line 32 is never reached.
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.
Sorry, my first suggestion is also incorrect. The fixed version:
if (lhs == nullptr)
return rhs == nullptr;
else if (rhs == nullptr)
return false;
return *lhs == *rhs;
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.
yep trying similar now. seems to work. this one still fails though, but this time with a SEGV #3959 rather than infinite loop.
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.
Yes, it's correct. Didn't get update in time.
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.
pushed the fix. thanks for finding! I updated trace diff for failing test: #3959
Fixed while doing #3817 Also see @kaby76's fix that I implemented: antlr/grammars-v4#2909