7903132: Replace casts with type test pattern #5

bowbahdoe · 2022-03-24T22:48:41Z

Replaces a handful of casts following type tests with the equivalent pattern syntax.

I cannot create a matching issue, as I do not have an account on the bug tracker.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed

Issue

CODETOOLS-7903132: Replace casts with type test pattern

Reviewers

Maurizio Cimadamore (@mcimadamore - Committer)
Jorn Vernee (@JornVernee - no project role)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5

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

Using diff file

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

bridgekeeper · 2022-03-24T22:48:50Z

👋 Welcome back bowbahdoe! 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.

mcimadamore · 2022-03-25T10:49:07Z

Are you in a position to file a JBS issue for this? We're happy to sponsor - the problem here is that the subject of the PR has to be in the form:

NNNNNNN:

For it to go through. If you are having issues, please let us know and we'll try to find an alternate arrangement.

bowbahdoe · 2022-03-25T14:38:57Z

My understanding is that I can't file a JBS issue until I get an account, which only happens after having made two contributions and petitioning for becoming an author.

I can do it the long way around certainly by filing a bug report via the oracle site, but I don't think this is the sort of thing that would make it past that filter.

JornVernee · 2022-03-25T14:57:55Z

I've filed an issue for this PR here: https://bugs.openjdk.java.net/browse/CODETOOLS-7903132 Please update the PR title to include the issue number.

JornVernee · 2022-03-25T15:04:05Z

src/main/java/org/openjdk/jextract/clang/Cursor.java

            return false;
        }
-        return (Index_h.clang_equalCursors(cursor, ((Cursor)other).cursor) != 0);
+        return (Index_h.clang_equalCursors(cursor, otherCursor.cursor) != 0);


I think in these cases we can do better by merging the instanceof check above this in the condition. i.e.

Suggested change

return (Index_h.clang_equalCursors(cursor, otherCursor.cursor) != 0);

return other instanceof Cursor otherCursor

&& (Index_h.clang_equalCursors(cursor, otherCursor.cursor) != 0);

And similar for other places that are computing a boolean value, such as all the equals methods.

Strictly speaking, the whole method body could be merged into one expression. Its the realm of infinite bikesheds though, and I did not / do not know if there is a set of implicit style choices there.

Well, we're on the topic of style changes already, so let's pull the string. This is new territory, so I think this is where we decide this :)

Merging the instanceof into the expression avoids having the pattern variable be carried over to outside of the if block, which I think is better.

The this == other check above that doesn't have the same problem.

I don't have any particularly strong opinions there, so I made that as the change, leaving the this == other checks alone

JornVernee · 2022-03-25T15:08:53Z

src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java

@@ -56,8 +56,7 @@

    ClassSourceBuilder(JavaSourceBuilder enclosing, Kind kind, String name) {
        this.enclosing = enclosing;
-        this.align = (enclosing instanceof ClassSourceBuilder) ?
-            ((ClassSourceBuilder) enclosing).align : 0;
+        this.align = (enclosing instanceof ClassSourceBuilder enclosing) ? enclosing.align : 0;


This is resulting in a compilation error:

jextract\src\main\java\org\openjdk\jextract\impl\ClassSourceBuilder.java:59: error: variable enclosing is already defined in constructor ClassSourceBuilder(JavaSourceBuilder,Kind,String) this.align = (enclosing instanceof ClassSourceBuilder enclosing) ? enclosing.align : 0;

Please build and test the patch locally on your machine as well, if you haven't done so already. (This should be done before suggesting changes).

So yes, 100% on me. But in my defense I am getting a particularly obtuse error before compilation and there isn't exactly someone to ask without pretense.

$ /usr/bin/clang --version Apple clang version 13.1.6 (clang-1316.0.21.2) Target: arm64-apple-darwin21.3.0 Thread model: posix InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin --------------------- $ sh ./gradlew -Pjdk18_home=/Users/emccue/Library/Java/JavaVirtualMachines/openjdk-18/Contents/Home clean verify FAILURE: Build failed with an exception. * Where: Build file '/Users/emccue/Development/jextract/build.gradle' line: 13 * What went wrong: A problem occurred evaluating root project 'jextract'. > Could not get unknown property 'libclang_home' for root project 'jextract' of type org.gradle.api.Project. * Try: > Run with --stacktrace option to get the stack trace. > Run with --info or --debug option to get more log output. > Run with --scan to get full insights. * Get more help at https://help.gradle.org BUILD FAILED in 297ms --------------------- $ sh ./gradlew -Pjdk18_home=/Users/emccue/Library/Java/JavaVirtualMachines/openjdk-18/Contents/Home -Plibclang_home=/usr/bin/clang clean verify FAILURE: Build failed with an exception. * Where: Build file '/Users/emccue/Development/jextract/build.gradle' line: 14 * What went wrong: A problem occurred evaluating root project 'jextract'. > Cannot invoke method getAt() on null object * Try: > Run with --stacktrace option to get the stack trace. > Run with --info or --debug option to get more log output. > Run with --scan to get full insights. * Get more help at https://help.gradle.org BUILD FAILED in 287ms --------------------- $ sh ./gradlew -Pjdk18_home=/Users/emccue/Library/Java/JavaVirtualMachines/openjdk-18/Contents/Home -Plibclang_home=/usr/bin/ clean verify FAILURE: Build failed with an exception. * Where: Build file '/Users/emccue/Development/jextract/build.gradle' line: 14 * What went wrong: A problem occurred evaluating root project 'jextract'. > Cannot invoke method getAt() on null object * Try: > Run with --stacktrace option to get the stack trace. > Run with --info or --debug option to get more log output. > Run with --scan to get full insights. * Get more help at https://help.gradle.org BUILD FAILED in 290ms

(I was counting on some CI/CD to be triggered on PR, which i suppose either isn't the case or isn't the case unless i get past the issue number check)

Questions can be directed at the mailing list (either jextract-dev@openjdk.java.net or panama-dev@openjdk.java.net). You can send an email there without making an account as well (it will go through a moderation queue first).

I agree the error message is not great. In this case the issue seems to be this line in build.gradle:

def clang_version = new File("${libclang_home}/lib/clang/").list()[0]

i.e. the libclang_home directory does not have the expected directory structure. It is looking for a directory like <libclang_home>/lib/clang/<VERSION>. So please make sure that exists.

On a stock mac install with XCode it ended up being located at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/ - in case that is of use to anyone.

openjdk · 2022-03-25T16:18:33Z

@bowbahdoe This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

7903132: Replace casts with type test pattern

Reviewed-by: mcimadamore, jvernee

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

71d7eae: Drop JBS check from jextract repo
60978d2: 7903129: jextract does not handle @argfile

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2022-03-25T16:22:33Z

Webrevs

mcimadamore

Looks good

bowbahdoe · 2022-03-25T23:08:46Z

/integrate

openjdk · 2022-03-25T23:09:14Z

@bowbahdoe
Your change (at version 63e2aa7) is now ready to be sponsored by a Committer.

mcimadamore · 2022-03-28T11:35:32Z

/sponsor

openjdk · 2022-03-28T11:37:06Z

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

fba7220: 7903133: add lp_solve sample
6018d1e: 7903134: Improve error reporting in jextract gradle build
71d7eae: Drop JBS check from jextract repo
60978d2: 7903129: jextract does not handle @argfile

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-28T11:37:24Z

@mcimadamore @bowbahdoe Pushed as commit 4c0e547.

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

Replace casts with type test pattern

0424713

JornVernee reviewed Mar 25, 2022

View reviewed changes

bowbahdoe changed the title ~~Replace casts with type test pattern~~ 7903132: Replace casts with type test pattern Mar 25, 2022

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 25, 2022

bowbahdoe added 2 commits March 25, 2022 15:48

Fix compile error with ClassSourceBuilder pattern

12c04e1

Use patterns inline with && expression when possible for equals

63e2aa7

mcimadamore approved these changes Mar 25, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 25, 2022

JornVernee approved these changes Mar 25, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 28, 2022

openjdk bot closed this Mar 28, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 28, 2022

bowbahdoe deleted the replace-casts-with-type-test-pattern branch March 29, 2022 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7903132: Replace casts with type test pattern #5

7903132: Replace casts with type test pattern #5

bowbahdoe commented Mar 24, 2022 •

edited by openjdk bot

bridgekeeper bot commented Mar 24, 2022

mcimadamore commented Mar 25, 2022

bowbahdoe commented Mar 25, 2022 •

edited

JornVernee commented Mar 25, 2022

JornVernee Mar 25, 2022

bowbahdoe Mar 25, 2022

JornVernee Mar 25, 2022 •

edited

bowbahdoe Mar 25, 2022

JornVernee Mar 25, 2022

bowbahdoe Mar 25, 2022 •

edited

bowbahdoe Mar 25, 2022

JornVernee Mar 25, 2022

bowbahdoe Mar 25, 2022

openjdk bot commented Mar 25, 2022 •

edited

mlbridge bot commented Mar 25, 2022 •

edited

mcimadamore left a comment

bowbahdoe commented Mar 25, 2022

openjdk bot commented Mar 25, 2022

mcimadamore commented Mar 28, 2022

openjdk bot commented Mar 28, 2022

openjdk bot commented Mar 28, 2022

	return (Index_h.clang_equalCursors(cursor, otherCursor.cursor) != 0);
	return other instanceof Cursor otherCursor
	&& (Index_h.clang_equalCursors(cursor, otherCursor.cursor) != 0);

7903132: Replace casts with type test pattern #5

7903132: Replace casts with type test pattern #5

Conversation

bowbahdoe commented Mar 24, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 24, 2022

mcimadamore commented Mar 25, 2022

bowbahdoe commented Mar 25, 2022 • edited

JornVernee commented Mar 25, 2022

JornVernee Mar 25, 2022

Choose a reason for hiding this comment

bowbahdoe Mar 25, 2022

Choose a reason for hiding this comment

JornVernee Mar 25, 2022 • edited

Choose a reason for hiding this comment

bowbahdoe Mar 25, 2022

Choose a reason for hiding this comment

JornVernee Mar 25, 2022

Choose a reason for hiding this comment

bowbahdoe Mar 25, 2022 • edited

Choose a reason for hiding this comment

bowbahdoe Mar 25, 2022

Choose a reason for hiding this comment

JornVernee Mar 25, 2022

Choose a reason for hiding this comment

bowbahdoe Mar 25, 2022

Choose a reason for hiding this comment

openjdk bot commented Mar 25, 2022 • edited

mlbridge bot commented Mar 25, 2022 • edited

Webrevs

mcimadamore left a comment

Choose a reason for hiding this comment

bowbahdoe commented Mar 25, 2022

openjdk bot commented Mar 25, 2022

mcimadamore commented Mar 28, 2022

openjdk bot commented Mar 28, 2022

openjdk bot commented Mar 28, 2022

bowbahdoe commented Mar 24, 2022 •

edited by openjdk bot

bowbahdoe commented Mar 25, 2022 •

edited

JornVernee Mar 25, 2022 •

edited

bowbahdoe Mar 25, 2022 •

edited

openjdk bot commented Mar 25, 2022 •

edited

mlbridge bot commented Mar 25, 2022 •

edited