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

Remove unnecessary branch from ReadTag #7289

Merged

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Mar 9, 2020

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.

  • What it does is that if we just read a tag that that is immediately followed by the limit of the stream (but the tag itself is still readable just fine) we will artificially return a tag of 0 and there seems to be no reason we we would want that
  • this also potentially affects performance (one extra check after reading a tag).
  • if I remove the check, the tests still seem to be passing.
if (ReachedLimit)
{
    return 0;
}

The snippet was by #5183
6f73c50#diff-1d948236189c1eb142de5ed1ed631a40R386

@jtattermusch
Copy link
Contributor Author

@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).

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Mar 9, 2020

CC @jskeet @JamesNK @anandolee

@ObsidianMinor
Copy link
Contributor

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.

@jtattermusch
Copy link
Contributor Author

@ObsidianMinor the development of that snippet is a bit more interesting:
First you added this in 8c13fe7#diff-1d948236189c1eb142de5ed1ed631a40R387,
after that you added the ReachedTagLimit condition here:
1048bf0#diff-1d948236189c1eb142de5ed1ed631a40R387
and finally you removed the ReachedTagLimit, but left the original snippet
97b7da7#diff-1d948236189c1eb142de5ed1ed631a40R386

But looks like we can safely remove that, do you agree?

@ObsidianMinor
Copy link
Contributor

Yes it's a useless check that should've been remove back then.

@jtattermusch
Copy link
Contributor Author

@anandolee this should be now ready to merge.

@anandolee anandolee merged commit a0cc0e8 into protocolbuffers:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants