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

Get tests passing with frozen-string-literals enabled. #354

Closed
wants to merge 2 commits into from

Conversation

pat
Copy link
Contributor

@pat pat commented Jun 22, 2017

These changes ensure that all string literals can be frozen (as per the optional feature in MRI 2.3 and onwards). I would recommend adding the following to your .travis.yml file to ensure regressions aren't introduced:

before_script:
- if (ruby -e "exit RUBY_VERSION.to_f >= 2.4"); then export RUBYOPT="--enable-frozen-string-literal"; fi; echo $RUBYOPT

This will add the flag when the tests are run on MRI 2.4 or newer (while the feature was introduced in 2.3, it doesn't seem to work reliably until 2.4).

@@ -237,7 +237,7 @@ def coerce_encoding(string)
end

def clear_buffer
@buffer = ''
@buffer = ''.dup
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use String.new for these to avoid the extra allocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've recently discovered via @koic that Ruby isn't quite consistent with String.new. String.new.encoding is ASCII-8BIT, whereas String.new('').encoding (which doesn't avoid the extra allocation) is UTF-8.

I'm happy to change the PR to use String.new('') - I presume UTF-8 is the preferred encoding? - I've just found that some library maintainers (particularly Rails) prefer dup.

@whitequark
Copy link
Owner

I would recommend adding the following to your .travis.yml file to ensure regressions aren't introduced:

Please do so.

@whitequark
Copy link
Owner

Fails:

/home/travis/.rvm/gems/ruby-2.4.1/gems/racc-1.4.14/lib/racc/statetransitiontable.rb:214:in `mkmapexp': can't modify frozen String (RuntimeError)

@pat
Copy link
Contributor Author

pat commented Jun 23, 2017

Yeah, I noticed that too :(

There are PRs (for both 1.4.x and master branches) for frozen string literals in racc pending, though they've been dormant for a little while. I'm going to try to help get their builds passing when I have a spare moment, but that's probably not in the next 24 hours or so.
ruby/racc#80
ruby/racc#81

@pat
Copy link
Contributor Author

pat commented Jan 2, 2018

Hi all, just coming back to this PR after several months, and with a couple of thoughts:

  • the master branch of racc seems to be frozen-string-literal-friendly (but I've no idea if/when a new gem release will come out).
  • These days I'm actually preferring the pragma comment # frozen_string_literal: true at the top of each file, rather than the before_script option I've added to the travis configuration. The catch is that it becomes something that needs to be remembered every time new files are introduced into the project - but removes the need for dependencies to be completely frozen-string-literal-friendly.

I'm happy to re-jig this PR to take the latter approach if that's desired? Would be great to get this merged in, rather than just waiting hopefully for a new racc release.

@whitequark
Copy link
Owner

but I've no idea if/when a new gem release will come out

Ping @tenderlove?

I'm happy to re-jig this PR to take the latter approach if that's desired? Would be great to get this merged in, rather than just waiting hopefully for a new racc release.

Sure, we can even do both.

@pat
Copy link
Contributor Author

pat commented Jan 4, 2018

@whitequark I submitted a new PR with the pragma comments. This way at least things aren't reliant on dependencies (and I'm sure @tenderlove is either busy enough, or hopefully enjoying a bit of a break).

So, I figure this PR can be closed, but possibly revisited later if people are particularly keen to use the RUBYOPT env flag.

@pat pat closed this Jan 4, 2018
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.

None yet

3 participants