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

StringLiteral consumers may not be CR-aware #2163

headius opened this Issue Nov 9, 2014 · 3 comments


None yet
3 participants
Copy link

headius commented Nov 9, 2014

The old StrNode propagated CR from the parser into the eventual String. Our new IR was not doing so until @cheald added it in 1899c26 to fix test_slice in mri/ruby/test_string.rb. However, there may be (are likely) other consumers of StringLiteral that are just going straight for the ByteList.

We need to audit all consumers of StringLiteral and make sure they are made CR-aware.

The changes made in 1899c26 broke the JIT logic because that logic was not re-checking the CR of the string as it is being only checked it based on the operands. So for the moment I'm disabling the optimized ByteList logic in the JIT. That will need to be fixed and restored for this bug.

@headius headius added core jit ir labels Nov 9, 2014

@headius headius added this to the JRuby milestone Nov 9, 2014


This comment has been minimized.

Copy link

cheald commented Nov 9, 2014

There are a number of places where StringLiterals are created, as well, and CR information isn't passed into them, so both consumers and creators should probably be audited.

headius added a commit that referenced this issue Nov 9, 2014


This comment has been minimized.

Copy link

enebo commented Nov 10, 2014

CR is a weird beast but I think if we CODERANGE_UNKNOWN all internal String code should be able to cope with recalculating it.

@enebo enebo modified the milestones:, JRuby Feb 10, 2015


This comment has been minimized.

Copy link
Member Author

headius commented Apr 17, 2015

I did a review of all StringLiteral.byteList/getByteList consumers and fixed a few places (d2ea85f) where we weren't using the cr from StringLiteral. Calling this done.

@headius headius closed this Apr 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.