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

s///e doesn't parse the replacement part #110

Closed
haukex opened this issue Oct 4, 2020 · 4 comments
Closed

s///e doesn't parse the replacement part #110

haukex opened this issue Oct 4, 2020 · 4 comments

Comments

@haukex
Copy link

haukex commented Oct 4, 2020

As the title says, s///e doesn't parse the replacement part, which is a Perl expression. Therefore, non-standard.pm code is allowed in the replacement part. I'm not sure how difficult this is to implement or not, but perhaps it should be documented as a limitation for now.

@xsawyerx xsawyerx added bug and removed bug labels Oct 4, 2020
@xsawyerx
Copy link
Owner

xsawyerx commented Oct 4, 2020

I put the bug label and removed it because I don't think it's a bug.

Technically, the only reason it doesn't understand the regexp modifier as a separate unit is that it sees it as a lexeme, not a rule. I don't remember why all of these regex functions are defined as lexemes instead of rules, though... I wouldn't touch it again without enough tests, to be honest.

@haukex
Copy link
Author

haukex commented Oct 7, 2020

Yes, I noticed that a regex is returned in the AST as one lexeme, which means that code trying to use the AST couldn't pick apart regexes further. But since Guacamole's current focus seems to be on verifying Standard Perl for now, that's probably ok. Anyway, yes, I wouldn't really call it a bug either, but a limitation, so perhaps a mention in the docs for now?

@xsawyerx
Copy link
Owner

It'd be great to have either more tests of it or a PR to improve this. I think separating the regex vs. modifier is a good idea. For the regex parts, it would be useful, but it also depends on the Perl version.

@xsawyerx
Copy link
Owner

Closing this for now. When it comes up again, we'll raise a proper ticket.
(This should also not be considered a Q-like value, since it's the s keyword instead, but one issue at a time.)

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

No branches or pull requests

2 participants