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

Ruby File Generation Frozen String Literals #7888

Closed

Conversation

noahmatisoff
Copy link

@noahmatisoff noahmatisoff commented Sep 11, 2020

Adds a comment to the top of the files that are generated to enforce frozen string literals.

A pragma comment is an instruction for Ruby that when reading the file, it should treat it a certain way. They only impact that specific file, not any other files that it in turn loads. Seeking guidance from the team on if this should be an option passed when doing the code generation.

This only works for Ruby 2.3.0 and up, however it shouldn't have any adverse impact on versions prior to 2.3.0.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@noahmatisoff
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@haberman
Copy link
Member

What is the motivation for this? What will it help?

@noahmatisoff
Copy link
Author

It improves memory performance of strings by making them immutable by default, more can be read here.

@ryanseys
Copy link

Any update here?

Copy link
Member

@acozzette acozzette left a comment

Choose a reason for hiding this comment

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

@noahmatisoff Thank you for the pull request. This change looks great but we just need an update to the test here. It looks like it's just failing because there are some golden files that need to be updated.

@acozzette
Copy link
Member

@noahmatisoff I'm going to close this since it's been a few weeks with no action, but feel free to comment here if you would like to reopen this.

@acozzette acozzette closed this Mar 9, 2022
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