do not generate null checks for record patterns#1884
do not generate null checks for record patterns#1884vicente-romero-oracle wants to merge 3 commits intoopenjdk:bworldfrom
Conversation
|
👋 Welcome back vromero! A progress list of the required criteria for merging this PR into |
|
@vicente-romero-oracle 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| @Override | ||
| public void visitLetExpr(LetExpr tree) { | ||
| // don't recurse inside let expressions, they are backend artifacts | ||
| result = tree; |
There was a problem hiding this comment.
offline suggestion from Jan Lahoda:
this code will skip all let expressions which is too broad, as let expressions are generated also by switch expressions but could contain user code. For example there is no null check generated in this case:
void main() {
int i = switch (new Object()) {
default -> {
Object obj = null;
Object! nonNull = obj;
yield 0;
}
};
}
There was a problem hiding this comment.
Actually, I think we can remove this visitor. Sure, javac will add an extra, redundant null check for null-restricted type test patterns, but that's not incorrect (this check will never throw NPE because, if null, the pattern won't match -- thanks to your changes in TransPatterns). I think I prefer a redundant check than having to mark all LetExpr that might need recursion (as we will surely forget some).
Separately, we can work on removing redundant null checks -- e.g. for things that are co-compiled and such, in which case this will fall out for free.
|
superseded by: #1902 |
do not generate null checks for record patterns
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1884/head:pull/1884$ git checkout pull/1884Update a local copy of the PR:
$ git checkout pull/1884$ git pull https://git.openjdk.org/valhalla.git pull/1884/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1884View PR using the GUI difftool:
$ git pr show -t 1884Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1884.diff
Using Webrev
Link to Webrev Comment