8255005: Fix indentation levels in classFileParser.cpp #931

hseigel · 2020-10-29T13:02:04Z

Please review this very small change to fix indentation in classFileParser.cpp and move the check for the PermittedSubclasses attribute inside of the "if (major_version >= 16)" block.

Fix was tested with tiers 1 and 2 on Linux, Mac OSX, and Windows, and tiers 3-5 on Linux x64.

Thanks, Harold

Progress

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

Issue

JDK-8255005: Fix indentation levels in classFileParser.cpp

Reviewers

Lois Foltan (@lfoltan - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/931/head:pull/931
$ git checkout pull/931

bridgekeeper · 2020-10-29T13:02:43Z

👋 Welcome back hseigel! 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 · 2020-10-29T13:04:12Z

@hseigel The following label will be automatically applied to this pull request:

hotspot-runtime

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.

mlbridge · 2020-10-29T13:07:49Z

Webrevs

01: Full - Incremental (1155576)
00: Full (e6873a3)

lfoltan · 2020-10-29T16:05:37Z

src/hotspot/share/classfile/classFileParser.cpp

-              record_attribute_start = cfs->current();
-              record_attribute_length = attribute_length;
-              }
-            cfs->skip_u1(attribute_length, CHECK);


Minor nit, since records, permitted subclasses and an unknown attribute type all do a "cfs->skip_u1(attribute_length, CHECK);" as the last action, you could pull that out to the end of the else if (_major_version >= JAVA_16_VERSION) statement.

Thanks for looking at this. Please see latest commit that contains the changes you suggested.

lfoltan

One minor nit for you to consider. Overall looks good!

openjdk · 2020-10-29T16:07:07Z

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

8255005: Fix indentation levels in classFileParser.cpp

Reviewed-by: lfoltan, coleenp

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

e48016b: 8255565: [Vector API] Add missing format strings for extract instructs in x86.ad
2c7fc85: 8254972: Fix pretouch chunk calculations
d128191: 8253473: Javadoc clean up in HttpHandler, HttpPrincipal, HttpContext, and HttpsConfigurator
379ba80: 8255595: delay_to_keep_mmu passes wrong arguments to Monitor wait
1a89d68: 8255285: Move JVMFlag origins into a new enum JVMFlagOrigin
56eb5f5: 8255466: C2 crashes at ciObject::get_oop() const+0x0
5782a2a: 8254975: lambda proxy fails to access a protected member inherited from a split package
d5138d1: 8255604: java/nio/channels/DatagramChannel/Connect.java fails with java.net.BindException: Cannot assign requested address: connect
2a2fa13: 8255449: Improve the exception message of MethodHandles::permuteArguments
2a50c3f: 8241495: Make more compiler related flags available on a per method level
... and 4 more: https://git.openjdk.java.net/jdk/compare/579e50bb404a5a858299841803a1f99441630aa5...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.

lfoltan

Thanks for making that change.

coleenp · 2020-10-29T21:31:41Z

src/hotspot/share/classfile/classFileParser.cpp

-        } else if (_major_version >= JAVA_15_VERSION) {
-          // Check for PermittedSubclasses tag
-          if (tag == vmSymbols::tag_permitted_subclasses()) {
+        } else if (_major_version >= JAVA_16_VERSION) {


I don't understand why you changed it from >= 15 to >= 16.

It was changed to 16 because, starting with 16, Records is a new non-preview feature. So, the Record attribute is for class file version 60 or later. 60 is the major class file version for JDK-16.

Sealed types are still preview but require class file version 60.65535. Preview features must always have the latest major class file version (and minor version of 655353).

Okay, thank you for the explanation about preview features. Looks good to me.

coleenp · 2020-10-30T12:50:37Z

src/hotspot/share/classfile/classFileParser.cpp

-        } else if (_major_version >= JAVA_15_VERSION) {
-          // Check for PermittedSubclasses tag
-          if (tag == vmSymbols::tag_permitted_subclasses()) {
+        } else if (_major_version >= JAVA_16_VERSION) {


Okay, thank you for the explanation about preview features. Looks good to me.

hseigel · 2020-10-30T12:55:07Z

/integrate

openjdk · 2020-10-30T12:57:48Z

@hseigel Since your change was applied there have been 14 commits pushed to the master branch:

e48016b: 8255565: [Vector API] Add missing format strings for extract instructs in x86.ad
2c7fc85: 8254972: Fix pretouch chunk calculations
d128191: 8253473: Javadoc clean up in HttpHandler, HttpPrincipal, HttpContext, and HttpsConfigurator
379ba80: 8255595: delay_to_keep_mmu passes wrong arguments to Monitor wait
1a89d68: 8255285: Move JVMFlag origins into a new enum JVMFlagOrigin
56eb5f5: 8255466: C2 crashes at ciObject::get_oop() const+0x0
5782a2a: 8254975: lambda proxy fails to access a protected member inherited from a split package
d5138d1: 8255604: java/nio/channels/DatagramChannel/Connect.java fails with java.net.BindException: Cannot assign requested address: connect
2a2fa13: 8255449: Improve the exception message of MethodHandles::permuteArguments
2a50c3f: 8241495: Make more compiler related flags available on a per method level
... and 4 more: https://git.openjdk.java.net/jdk/compare/579e50bb404a5a858299841803a1f99441630aa5...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8a065ef.

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

Reviewed-by: lfoltan, coleenp

8255005: Fix indentation levels in classFileParser.cpp

e6873a3

openjdk bot added the rfr Pull request is ready for review label Oct 29, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 29, 2020

lfoltan reviewed Oct 29, 2020

View reviewed changes

lfoltan approved these changes Oct 29, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 29, 2020

8255005: Fix indentation levels in classFileParser.cpp

1155576

lfoltan approved these changes Oct 29, 2020

View reviewed changes

coleenp suggested changes Oct 29, 2020

View reviewed changes

coleenp approved these changes Oct 30, 2020

View reviewed changes

openjdk bot closed this Oct 30, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 30, 2020

openjdk-notifier bot referenced this pull request Oct 30, 2020

8255005: Fix indentation levels in classFileParser.cpp

8a065ef

Reviewed-by: lfoltan, coleenp

8255005: Fix indentation levels in classFileParser.cpp #931

8255005: Fix indentation levels in classFileParser.cpp #931

hseigel commented Oct 29, 2020 •

edited by openjdk bot

bridgekeeper bot commented Oct 29, 2020

openjdk bot commented Oct 29, 2020

mlbridge bot commented Oct 29, 2020 •

edited

lfoltan Oct 29, 2020

hseigel Oct 29, 2020

lfoltan left a comment

openjdk bot commented Oct 29, 2020 •

edited

lfoltan left a comment

coleenp Oct 29, 2020

hseigel Oct 30, 2020

coleenp Oct 30, 2020

coleenp Oct 30, 2020

hseigel commented Oct 30, 2020

openjdk bot commented Oct 30, 2020

8255005: Fix indentation levels in classFileParser.cpp #931

8255005: Fix indentation levels in classFileParser.cpp #931

Conversation

hseigel commented Oct 29, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Oct 29, 2020

openjdk bot commented Oct 29, 2020

mlbridge bot commented Oct 29, 2020 • edited

Webrevs

lfoltan Oct 29, 2020

Choose a reason for hiding this comment

hseigel Oct 29, 2020

Choose a reason for hiding this comment

lfoltan left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 29, 2020 • edited

lfoltan left a comment

Choose a reason for hiding this comment

coleenp Oct 29, 2020

Choose a reason for hiding this comment

hseigel Oct 30, 2020

Choose a reason for hiding this comment

coleenp Oct 30, 2020

Choose a reason for hiding this comment

coleenp Oct 30, 2020

Choose a reason for hiding this comment

hseigel commented Oct 30, 2020

openjdk bot commented Oct 30, 2020

hseigel commented Oct 29, 2020 •

edited by openjdk bot

mlbridge bot commented Oct 29, 2020 •

edited

openjdk bot commented Oct 29, 2020 •

edited