8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools #6599

vicente-romero-oracle · 2021-11-29T19:05:59Z

Hi,

Please review this PR which is basically rewriting some redundant boolean expressions in the compiler.

TIA

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools

Reviewers

Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6599/head:pull/6599
$ git checkout pull/6599

Update a local copy of the PR:
$ git checkout pull/6599
$ git pull https://git.openjdk.java.net/jdk pull/6599/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6599

View PR using the GUI difftool:
$ git pr show -t 6599

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6599.diff

….compiler and langtools

bridgekeeper · 2021-11-29T19:07:15Z

👋 Welcome back vromero! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-11-29T19:09:16Z

@vicente-romero-oracle The following labels will be automatically applied to this pull request:

build
compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-11-29T19:13:29Z

Webrevs

jddarcy · 2021-11-29T19:53:48Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java

@@ -1313,9 +1313,9 @@ public void visitConditional(JCConditional tree) {
                @Override
                public void visitReference(JCMemberReference tree) {
                    if (sRet.hasTag(VOID)) {
-                        result &= true;
+                        result = true;


Isn't the equivalent statement to
result &= true
just
result
?

Agreeing with jddarcy:
a & true === a
so current code just keeps the value of result while the new one sets result to true.

jddarcy · 2021-11-29T19:54:03Z

make/langtools/tools/compileproperties/CompileProperties.java

@@ -183,7 +183,7 @@ private boolean parseOptions(String args[]) {
                        log.error("cannot close " + filename, e);
                    }
                }
-                if ( ok = true && contents != null ) {
+                if ( ok && contents != null ) {
                    String tokens[] = (new String(contents)).split("\\s+");


So the intended composite predicate here is thought to be
ok == true && contents != null
which is equivalent to
ok && contents != null.
The semantics of the current code are equivalent to just
contents != null
right?

The semantics is definitely changed (considering the first branch was always true originally) but it may be the original implementation being incorrect,

well not assigning true to ok also breaks the semantics as ok is used in boolean expressions below this point. So if we want to keep the semantics 100% we will have to either let the code as it was or do:

ok = true; if (contents != null) { ...

JarvisCraft · 2021-11-30T00:09:27Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java

@@ -1313,9 +1313,9 @@ public void visitConditional(JCConditional tree) {
                @Override
                public void visitReference(JCMemberReference tree) {
                    if (sRet.hasTag(VOID)) {
-                        result &= true;
+                        result = true;


Agreeing with jddarcy:
a & true === a
so current code just keeps the value of result while the new one sets result to true.

JarvisCraft · 2021-11-30T00:09:27Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java

@@ -1335,9 +1335,9 @@ public void visitParens(JCParens tree) {
                @Override
                public void visitLambda(JCLambda tree) {
                    if (sRet.hasTag(VOID)) {
-                        result &= true;
+                        result = true;


Same as above, the change is changing the semantics:
original code keeps the value of result while the new one sets it to true.

JarvisCraft · 2021-11-30T00:09:27Z

make/langtools/tools/compileproperties/CompileProperties.java

@@ -183,7 +183,7 @@ private boolean parseOptions(String args[]) {
                        log.error("cannot close " + filename, e);
                    }
                }
-                if ( ok = true && contents != null ) {
+                if ( ok && contents != null ) {
                    String tokens[] = (new String(contents)).split("\\s+");


The semantics is definitely changed (considering the first branch was always true originally) but it may be the original implementation being incorrect,

magicus · 2021-11-30T12:47:08Z

/label remove build

openjdk · 2021-11-30T12:48:59Z

@magicus
The build label was successfully removed.

vicente-romero-oracle · 2021-12-06T16:20:23Z

submitted another iteration to the PR, thanks for the comment so far

lahodaj

lgtm

openjdk · 2021-12-07T15:24:56Z

@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:

8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools

Reviewed-by: jlahoda

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 320 new commits pushed to the master branch:

bb50b92: 8277536: Use String.blank in jdk.javadoc where applicable
5b81d5e: 8276901: Implement UseHeavyMonitors consistently
69d8669: 8278339: ServerSocket::isClosed may return false after accept throws
56ca66e: 8277863: Deprecate sun.misc.Unsafe methods that return offsets
3536127: 8277383: VM.metaspace optionally show chunk freelist details
44fcee3: 8278289: Drop G1BlockOffsetTablePart::_object_can_span
b2638e5: 8244602: Add JTREG_REPEAT_COUNT to repeat execution of a test
07669e3: 8275375: [REDO] JDK-8271949 dumppath in -XX:FlightRecorderOptions does not affect
24877ac: 8278270: ServerSocket is not thread safe
e535cb3: 8272395: Bad HTML in JVMTI man page
... and 310 more: https://git.openjdk.java.net/jdk/compare/fe45835f7cebfccd4544ae19d88bdc7f07560fbe...master

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 master branch, type /integrate in a new comment.

vicente-romero-oracle · 2021-12-07T17:23:29Z

thanks Jan for the approval :) I will wait a bit for the other reviewers

vicente-romero-oracle · 2021-12-07T23:29:28Z

/integrate

openjdk · 2021-12-07T23:31:17Z

Going to push as commit 3c2951f.
Since your change was applied there have been 332 commits pushed to the master branch:

3d61372: 8278363: Create extented container test groups
716c2e1: 8278368: ProblemList tools/jpackage/share/MultiNameTwoPhaseTest.java on macosx-x64
a8a1fbc: 8278068: Fix next-line modifier (snippet markup)
061017a: 8273175: Add @SInCE tags to the DocTree.Kind enum constants
d7c283a: 8275233: Incorrect line number reported in exception stack trace thrown from a lambda expression
3955b03: 8277328: jdk/jshell/CommandCompletionTest.java failures on Windows
5a036ac: 8277990: NMT: Remove NMT shutdown capability
7217cb7: 8274883: (se) Selector.open throws IAE when the default file system provider is changed to a custom provider
7ea4b19: 8278166: java/nio/channels/Channels/TransferTo.java timed out
543d1a8: 8275721: Name of UTC timezone in a locale changes depending on previous code
... and 322 more: https://git.openjdk.java.net/jdk/compare/fe45835f7cebfccd4544ae19d88bdc7f07560fbe...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-07T23:31:33Z

@vicente-romero-oracle Pushed as commit 3c2951f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8275771: JDK source code contains redundant boolean operations in jdk…

4fccdf4

….compiler and langtools

openjdk bot added the rfr label Nov 29, 2021

openjdk bot added build compiler labels Nov 29, 2021

jddarcy reviewed Nov 29, 2021

View changes

JarvisCraft suggested changes Nov 30, 2021

View changes

openjdk bot removed the build label Nov 30, 2021

addressing review comments

012779d

addressing review comments

d009c43

lahodaj approved these changes Dec 7, 2021

View changes

openjdk bot added the ready label Dec 7, 2021

openjdk bot closed this Dec 7, 2021

openjdk bot added integrated and removed ready rfr labels Dec 7, 2021

openjdk / jdk Public

8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools #6599

8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools #6599

vicente-romero-oracle commented Nov 29, 2021 •

edited by openjdk[bot] bot

bridgekeeper bot commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

mlbridge bot commented Nov 29, 2021 •

edited

jddarcy Nov 29, 2021

JarvisCraft Nov 30, 2021

jddarcy Nov 29, 2021

JarvisCraft Nov 30, 2021

vicente-romero-oracle Dec 3, 2021 •

edited

JarvisCraft Nov 30, 2021

JarvisCraft Nov 30, 2021

JarvisCraft Nov 30, 2021

magicus commented Nov 30, 2021

openjdk bot commented Nov 30, 2021

vicente-romero-oracle commented Dec 6, 2021

lahodaj left a comment

openjdk bot commented Dec 7, 2021 •

edited

vicente-romero-oracle commented Dec 7, 2021

vicente-romero-oracle commented Dec 7, 2021

openjdk bot commented Dec 7, 2021

openjdk bot commented Dec 7, 2021

openjdk / jdk Public

8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools #6599

8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools #6599

Conversation

vicente-romero-oracle commented Nov 29, 2021 • edited by openjdk[bot] bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

mlbridge bot commented Nov 29, 2021 • edited

Webrevs

jddarcy Nov 29, 2021

Choose a reason for hiding this comment

JarvisCraft Nov 30, 2021

Choose a reason for hiding this comment

jddarcy Nov 29, 2021

Choose a reason for hiding this comment

JarvisCraft Nov 30, 2021

Choose a reason for hiding this comment

vicente-romero-oracle Dec 3, 2021 • edited

Choose a reason for hiding this comment

JarvisCraft Nov 30, 2021

Choose a reason for hiding this comment

JarvisCraft Nov 30, 2021

Choose a reason for hiding this comment

JarvisCraft Nov 30, 2021

Choose a reason for hiding this comment

magicus commented Nov 30, 2021

openjdk bot commented Nov 30, 2021

vicente-romero-oracle commented Dec 6, 2021

lahodaj left a comment

openjdk bot commented Dec 7, 2021 • edited

vicente-romero-oracle commented Dec 7, 2021

vicente-romero-oracle commented Dec 7, 2021

openjdk bot commented Dec 7, 2021

openjdk bot commented Dec 7, 2021

vicente-romero-oracle commented Nov 29, 2021 •

edited by openjdk[bot] bot

mlbridge bot commented Nov 29, 2021 •

edited

vicente-romero-oracle Dec 3, 2021 •

edited

openjdk bot commented Dec 7, 2021 •

edited