-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
Remove unnecessary branch from ReadTag #7289
Remove unnecessary branch from ReadTag #7289
Conversation
@ObsidianMinor can you provide an explanation why the snippet is important? (and provide a test case that demonstrates it). FYI, the code change is just removing the snippet, the other lines is just adjusting CR-LF line endings (done automatically by the editor). |
This seems to be a mistake left over from when the CodedInputStream tracked groups. On my original proto2 prototype it calls ReachedTagLimit in that position but some way or another it got changed and then never got removed when we later moved the check for the end tag into the group message itself. |
@ObsidianMinor the development of that snippet is a bit more interesting: But looks like we can safely remove that, do you agree? |
Yes it's a useless check that should've been remove back then. |
a2f7c54
to
b4407b4
Compare
@anandolee this should be now ready to merge. |
I found this when doing some refactorings to how parsing code works.
The purpose of the following snipped at the end of CodedInputStream.ReadTag is unclear.
I don't see a reason why this would be necessary and it actually seems wrong.
The snippet was by #5183
6f73c50#diff-1d948236189c1eb142de5ed1ed631a40R386