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 'u' suffix support from TextFormat. #1642

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

thomasvl
Copy link
Collaborator

https://protobuf.dev/reference/protobuf/textformat-spec/#numeric and https://protobuf.dev/reference/protobuf/textformat-spec/#literals don't seem to show any signs that a 'u' suffix was ever valid. I also don't see any references in the C++ code. It also seems like we would parse -12u as valid which seems a little odd since it clearly isn't an unsigned value.

So dropping the 'u' support as it seems out of spec.

https://protobuf.dev/reference/protobuf/textformat-spec/#numeric and
https://protobuf.dev/reference/protobuf/textformat-spec/#literals don't seem to
show any signs that a 'u' suffix was ever valid. I also don't see any references
in the C++ code. It also seems like we would parse `-12u` as valid which seems a
little odd since it clearly isn't an unsigned value.

So dropping the 'u' support as it seems out of spec.
@thomasvl thomasvl requested a review from tbkka April 29, 2024 14:14
@thomasvl
Copy link
Collaborator Author

@tbkka - I haven't been able to figure out where this support came from, but it doesn't seem to be to spec. This isn't an api change, but it is a behavior change, so if you'd rather keep it unless we do want to do breaking changes, we can just add a comment that it isn't to spec and it should be removed in the future. Up to you.

@tbkka
Copy link
Collaborator

tbkka commented Apr 29, 2024

An early version of this file (commit 0aa5109 from Dec 2016) has a comment:

                        case "f", "u":
                            // proto1 allowed floats to be suffixed with 'f'
                            // and unsigned integers to be suffixed with 'u'
                            // Just ignore it:
                            return .number(s)

If I understand correctly, proto1 was never used outside of Google and I presume is no longer used inside of Google, so I'm fine just removing this.

@thomasvl
Copy link
Collaborator Author

Ok, since the C++ doesn't support it either, then, yea, guess safe to drop.

Did you want to go ahead and stamp this with approval?

@thomasvl thomasvl merged commit 1881999 into apple:main Apr 29, 2024
10 of 11 checks passed
@thomasvl thomasvl deleted the text_format_u_suffix branch April 29, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants