-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8361494: [IR Framework] Escape too much in replacement of placeholder #26243
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
8361494: [IR Framework] Escape too much in replacement of placeholder #26243
Conversation
👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into |
@marc-chevalier This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 28 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@marc-chevalier The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
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 implementing this improvement @marc-chevalier! Nice to see this paper cut getting eliminated.
I have a small suggestion, but this looks good to me regardless.
@@ -62,7 +62,7 @@ public String regex(CompilePhase compilePhase, VMInfo vmInfo, Comparison.Bound b | |||
if (IRNode.isVectorIRNode(node)) { | |||
nodeRegex = regexForVectorIRNode(nodeRegex, vmInfo, bound); | |||
} else if (userPostfix.isValid()) { | |||
nodeRegex = nodeRegex.replaceAll(IRNode.IS_REPLACED, userPostfix.value()); | |||
nodeRegex = nodeRegex.replaceAll(IRNode.IS_REPLACED, java.util.regex.Matcher.quoteReplacement(userPostfix.value())); |
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.
Perhaps you might want to import java.util.regex.Matcher
to make it a bit more concise?
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.
Yes, please import it instead.
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.
Good improvement! Thanks for fixing it.
@@ -62,7 +62,7 @@ public String regex(CompilePhase compilePhase, VMInfo vmInfo, Comparison.Bound b | |||
if (IRNode.isVectorIRNode(node)) { | |||
nodeRegex = regexForVectorIRNode(nodeRegex, vmInfo, bound); | |||
} else if (userPostfix.isValid()) { | |||
nodeRegex = nodeRegex.replaceAll(IRNode.IS_REPLACED, userPostfix.value()); | |||
nodeRegex = nodeRegex.replaceAll(IRNode.IS_REPLACED, java.util.regex.Matcher.quoteReplacement(userPostfix.value())); |
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.
Yes, please import it instead.
Imported. Could you take a look again? |
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 copyright in RawIRNode.java
and TestMergeStores.java
needs updating.
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 addressing the comments. Looks good.
Thanks @mhaessig! @chhagedorn, I still need a the almighty powers of a reviewer. |
/integrate Thanks @mhaessig and @chhagedorn! |
Going to push as commit 76442f3.
Your commit was automatically rebased without conflicts. |
@marc-chevalier Pushed as commit 76442f3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
In
RawIRNode::regex
, the call toString::replaceAll
doesn't quote the replace string.Meaning that in the IR rule
The interpreted
\w
is interpreted as a group reference, and we getso we should write instead
To mean the interpreted string
\\w
, to mean an escaped single backslash. Same goes with$
(used for nested classes).Since we don't want to refer to groups (and anyway, there are not in
IRNode.IS_REPLACED
), we just quote the replacement string withjava.util.regex.Matcher.quoteReplacement
to make it more usable.Note that you would still need to write
\\$
since the$
is the end of string regex, and needs to be escaped at the regex level (and not at the string, so it's not\$
, since$
is not a special character). Before the fix, it should be\\\\$
. Phew! Regexes are bad enough, let's not escape them manually twice!In
test/hotspot/jtreg/compiler/c2/TestMergeStores.java
, that makes us save 1344 backslashes.Thanks,
Marc
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26243/head:pull/26243
$ git checkout pull/26243
Update a local copy of the PR:
$ git checkout pull/26243
$ git pull https://git.openjdk.org/jdk.git pull/26243/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26243
View PR using the GUI difftool:
$ git pr show -t 26243
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26243.diff
Using Webrev
Link to Webrev Comment