Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cleaned up generated code for closed Enums #1751

Merged
merged 3 commits into from
Feb 5, 2025
Merged

Conversation

nicked
Copy link
Contributor

@nicked nicked commented Feb 1, 2025

For closed enums there no special UNRECOGNIZED case, so a plain Swift Int enum with explicit values can be used directly.

This results in the generated init?(rawValue:) and var rawValue functions not being needed, since the compiler will create them automatically.

@thomasvl
Copy link
Collaborator

thomasvl commented Feb 4, 2025

Huh, never really thought out this; since the open enums needed it we didn't consider if the closed could avoid the extra generation.

@tbkka any thoughts?

@thomasvl thomasvl requested a review from tbkka February 4, 2025 14:30
Copy link
Collaborator

@tbkka tbkka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice simplification.

@tbkka tbkka added the semver/none No version bump required. label Feb 4, 2025
@thomasvl
Copy link
Collaborator

thomasvl commented Feb 4, 2025

Looks like the PR needs the Reference directory updated also for the plugin tests.

@nicked
Copy link
Contributor Author

nicked commented Feb 4, 2025

Hi, any idea why protobuf itself would fail to build?

@thomasvl
Copy link
Collaborator

thomasvl commented Feb 4, 2025

Hi, any idea why protobuf itself would fail to build?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90415
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=14597b680a24b6f7375e4470dea935da9c369feb

Appears to be a compiler bug. I've let the protobuf team upstream know, hopefully they can tweak their sources to avoid this since it will be an issue for their customers on older (but supported) versions.

@nicked
Copy link
Contributor Author

nicked commented Feb 4, 2025

Think I've got the workflow figured out now, thanks for your help!

@thomasvl
Copy link
Collaborator

thomasvl commented Feb 5, 2025

Looks like an upstream fix is coming: protocolbuffers/protobuf#20234

@thomasvl
Copy link
Collaborator

thomasvl commented Feb 5, 2025

And with a respin everything is happy.

Thanks for noticing this, merging.

@thomasvl thomasvl merged commit 4f6e091 into apple:main Feb 5, 2025
13 checks passed
@nicked nicked deleted the enum-cleanup branch February 5, 2025 15:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants