-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add 'E' modifier to Regex for :export opt #14907
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
Conversation
1e87812 to
b85fed9
Compare
b85fed9 to
64220bf
Compare
|
Sorry still need to handle the OTP27- cases and fix the CI on this better, putting this back to draft but the approach can already be discussed. |
|
@sabiwara love this. I think this is the way to go. Preferrable we can improve the code in |
lib/elixir/lib/regex.ex
Outdated
| * `:ungreedy` (U) - inverts the "greediness" of the regexp | ||
| (the previous `r` option is deprecated in favor of `U`) | ||
| * `:export` (e) (from Erlang OTP 28.1 and Elixir 1.20) - uses an exported pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * `:export` (e) (from Erlang OTP 28.1 and Elixir 1.20) - uses an exported pattern | |
| * `:export` (E) (from Erlang OTP 28.1 and Elixir 1.20) - uses an exported pattern |
e is already used by Perl/PHP and has another meaning, let's use E to avoid any confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing the research, I thought about looking but I forgot.
ef34268
lib/elixir/lib/kernel.ex
Outdated
| # We defer to a runtime compilation if using the :export flag on older OTP version | ||
| # that don't support it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for earlier versions, we should always consider as if export is given, given they can be transmited across nodes and written to config files? So earlier versions don't need to check for it?
Also, why do we want to skip compilation if the "E" flag is given? We can still precompile it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that it would make the compilation fail because of the :export flag: https://github.com/sabiwara/elixir/actions/runs/19327180274/job/55281069375
What we could do is make E a non-op on older versions, e.g. just filter out the flag. ~r/foo/E == ~r/foo/.
This way, code written with E would be compatible on both older versions and newer versions.
It might be a bit confusing, but we could document it and it is only a transitional state of things until we drop OTP27-, so LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we could do is make E a non-op on older versions
Yes, I'd go this route. We can note that E is a no-op in older versions as all regexes are "exportable" there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think I got it: ddfc080
Something like 1b009af? |
|
Yes, beautiful. I believe we do similar traversal for releases too... perhaps this could be unified somewhere even. |
Unifying sounds good indeed. I'm not too familiar with releases, any pointers on where I should look? |
|
@sabiwara check |
|
@josevalim the structure is a bit different in releases: we're just validating terms with a bool function and returning terms: elixir/lib/mix/lib/mix/release.ex Lines 486 to 503 in 0126f1f
But perhaps the easy way here is to just modify the error message to mention regexes? Also, I'm wondering. For libraries that want to support various versions of Elixir and Erlang and use regex configs, it might be convenient to automatically export regexes from the config even if they don't have the E modifier? (so we don't get a compile error on Elixir 1.19.2/OTP27 that E doesn't exist). |
|
@sabiwara let's skip the config change for now then, we can tackle it later! This is good to go IMO! |
Close #14648
This is a proposal to address the fact regexes can't be shared across nodes or through config, just by exposing the already existing
:exportoption as a built-in modifiere. No need to fully recompile, we can just re-import (which we're doing already actually).Maybe we could also have
Regex.recompile!strip the:exportoption to get back, or expose a way to do this?