Add specs for regex literal expansion #13253
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
straight-shoota
commented
Apr 3, 2023
| end | ||
|
|
||
| private def regex_options(node) | ||
| Call.new(Path.global(["Regex", "Options"]).at(node), "new", NumberLiteral.new(node.options.value.to_s).at(node)).at(node) |
There was a problem hiding this comment.
The to_s is necessary to avoid a tight coupling between the value type of the compiler and stdlib. The base type of Regex::Options was changed to UInt64 in #13223.
straight-shoota
added
kind:bug
HertzDevil
approved these changes
Apr 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a useful addition in general and particularly prepares for #13252
Edit: Also discovers and fixes a bug (?) introduced in #13223.
Previous to #13223, the base type of
Regex::Optionswas the defaultInt32. That PR changed it toUInt64to gain more space.The parser creates an instance of
Regex::Optionsfrom the modifier flags of a regex literal. When the compiler expands them, the option value is expressed by the base type value of theRegex::Optionstype. The resulting number literal is typed, so the expression for theMULTILINEoptions value would change from::Regex::Options.new(6)to::Regex::Options.new(6_u64).Now this isn't an immediate problem when the base type of
Regex::Optionsin the compiler and stdlib align. And even if they don't, autocasting of integer literals should allow the generated code to be compatible with olderRegex::Optionsimplementations withInt32base type.Still, I think it's better to avoid such unintended side effects and keep the generated code consistent until we explicitly chose to change it for good as discussed in #13252.
The different domain range of
Int32andUInt64is irrelevant for this because the compiler only handles the three modifiers and the biggest integer value needed to represent them is15(forimx).