Skip to content
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

JDK-8276039: Remove unnecessary qualifications of java_lang_Class:: #6130

Closed
wants to merge 2 commits into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Oct 27, 2021

Please review,

I have no idea why this problem has not been encountered in compilation, it is obvious the usage of resolution operator :: is not complete and wrong.

Tests: mach5 tier1, tier4

Thanks
Yumin


Progress

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

Issue

  • JDK-8276039: Remove unnecessary qualifications of java_lang_Class::

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6130

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 27, 2021

👋 Welcome back minqi! 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
Copy link

@openjdk openjdk bot commented Oct 27, 2021

@yminqi 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.

@openjdk openjdk bot added the hotspot-runtime label Oct 27, 2021
@yminqi yminqi marked this pull request as ready for review Oct 27, 2021
@openjdk openjdk bot added the rfr label Oct 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 27, 2021

Webrevs

vidmik
vidmik approved these changes Oct 27, 2021
Copy link
Contributor

@vidmik vidmik left a comment

That's funny. I guess java_lang_Class: becomes a label and set_init_lock is local to the class so resolution works anyway.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2021

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

8276039: Remove unnecessary qualifications of java_lang_Class::

Reviewed-by: mikael, iklam

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

  • 63b9f8c: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert.
  • cb989cf: 8275052: AArch64: Severe AES/GCM slowdown on MacOS for short blocks
  • c92f230: 8276110: Problemlist javax/swing/JMenu/4515762/bug4515762.java for macos12
  • 309acbf: 8275703: System.loadLibrary fails on Big Sur for libraries hidden from filesystem
  • abe52ae: 8275518: accessibility issue in Inet6Address docs
  • 85d8cd8: 8276100: Remove G1SegmentedArray constructor name parameter
  • a343fa8: 8275865: Print deoptimization statistics in product builds
  • bec977c: 8275917: Some locks shouldn't allow_vm_block
  • 7c996d5: 8269401: Merge "Exceptions" and "Errors" into "Exception Classes"
  • d88b89f: 8276067: ZGC: Remove unused function alloc_high_address_at_most()
  • ... and 17 more: https://git.openjdk.java.net/jdk/compare/d98b7c25910d38ac644838f59cb41ecd131c87a9...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.

@openjdk openjdk bot added the ready label Oct 27, 2021
@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Oct 27, 2021

@vidmik is right - java_lang_Class: is just a label on the call to set_init_lock. No resolution is needed so the right fix here is to delete the "label" and just leave set_init_lock.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Delete the "label"

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 27, 2021

Delete the "label"

There are other similarity for static members like this in the same file:
1007: java_lang_Class::set_klass(mirror(), k);
1012: java_lang_Class::set_static_oop_field_count(mirror(), mk->compute_static_oop_field_count(mirror()));
1041: java_lang_Class::set_klass(mirror(), NULL);
1131: assert(java_lang_Class::is_instance(mirror_obj), "must be");
1245: oop comp_mirror = java_lang_Class::component_mirror(mirror);
1257: java_lang_Class::set_component_mirror(archived_mirror, archived_comp_mirror);
1264: java_lang_Class::set_init_lock(archived_mirror, NULL);
1377: java_lang_Class::set_module(k->java_mirror(), module());
1465: o = StringTable::intern(java_lang_Class::as_external_name(java_class()), THREAD);
1492: assert(java_lang_Class::static_oop_field_count(java_class) == 0, "should have been zeroed by allocation");
1498: assert(java_lang_Class::is_instance(java_class), "must be a Class object");
1504: assert(java_lang_Class::is_instance(java_class), "must be a Class object");
1524: assert(java_lang_Class::is_instance(java_class), "must be a Class object");
1555: assert(java_lang_Class::is_instance(java_class), "must be a Class object");
1582: assert(java_lang_Class::is_primitive(java_class), "just checking");
1596: assert(java_lang_Class::is_instance(java_class), "must be a Class object");
1612: assert(java_lang_Class::is_primitive(mirror), "must be primitive");
2263: InstanceKlass* holder = InstanceKlass::cast(java_lang_Class::as_Klass(mirror()));
2633: InstanceKlass* holder = InstanceKlass::cast(java_lang_Class::as_Klass(bte._mirror()));
2706: InstanceKlass* holder = InstanceKlass::cast(java_lang_Class::as_Klass(bte._mirror()));
2745: oop classname = java_lang_Class::name(java_class, CHECK);
2799: source_file = java_lang_Class::source_file(java_class());
2805: java_lang_Class::set_source_file(java_class(), source_file);
2811: java_lang_Class::set_source_file(java_class(), source_file);
2885: Klass* clazz = java_lang_Class::as_Klass(java_lang_invoke_MemberName::clazz(mname()));
3485: Klass* k = java_lang_Class::as_Klass(mirror);
4090: java_lang_Class::print_signature(pts->obj_at(i), st);
4093: java_lang_Class::print_signature(rtype(mt), st);
4148: BasicType bt = java_lang_Class::as_BasicType(pts->obj_at(i));
4155: BasicType bt = java_lang_Class::as_BasicType(rtype(mt));

I would like to keep this as others --- maybe the original author like it used this way.

iklam
iklam approved these changes Oct 27, 2021
Copy link
Member

@iklam iklam left a comment

This looks good and trivial to me.

For me the bug title is a little difficult to understand. How about changing it to something like:

typo: java_lang_Class:set_init_lock() should be java_lang_Class::set_init_lock()

@yminqi yminqi changed the title JDK-8276039: Incomplete resolution operator for set_init_lock in javaClasses.cpp JDK-8276039: typo: java_lang_Class:set_init_lock() should be java_lang_Class::set_init_lock() Oct 27, 2021
@@ -1261,7 +1261,7 @@ oop java_lang_Class::process_archived_mirror(Klass* k, oop mirror,
// Reset local static fields in the mirror
InstanceKlass::cast(k)->do_local_static_fields(&reset);

java_lang_Class:set_init_lock(archived_mirror, NULL);
java_lang_Class::set_init_lock(archived_mirror, NULL);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than fixing the qualification, I would just drop the qualification completely, being consistent with most of the other similar calls in this function, including several that immediately follow.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the list of qualified uses given earlier as examples, roughly half are required because they are outside the scope of java_lang_Class, and roughly half are unnecessary. This one is unnecessary, and qualifying it makes it different from adjacent usage.

Copy link
Contributor Author

@yminqi yminqi Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, not all of them are unnecessary. I just grep from the file and checked part of them, thanks! I will remove the unnecessary qualifier 'java_lang_Class::' for static functions defined in java_lang_Class.

Copy link
Contributor Author

@yminqi yminqi Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact class java_lang_Thread, java_lang_String maybe other classes have same usage. I will only change for class java_lang_Class in this PR.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Removing all unnecessary qualifications from java_lang_Class is a fine cleanup to do, but the issue title should now be changed to reflect that. You can use the discovery of the typo as the motivation for the broader cleanup.

Thanks,
David

@yminqi yminqi changed the title JDK-8276039: typo: java_lang_Class:set_init_lock() should be java_lang_Class::set_init_lock() JDK-8276039: Remove unnecessary qualifications of java_lang_Class:: Oct 28, 2021
@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 29, 2021

@vidmik @iklam Thanks for review
@dholmes-ora @kimbarrett Thanks for comment
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 29, 2021

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

  • 13265f9: 8274750: java/io/File/GetXSpace.java failed: '/dev': 191488 != 190976
  • 5facaa2: 8276128: (bf) Remove unused constant ARRAY_BASE_OFFSET from Direct-X-Buffer
  • d6d82f5: 8275608: runtime/Metaspace/elastic/TestMetaspaceAllocationMT2 too slow
  • a1ec4f9: 6854300: [TEST_BUG] java/awt/event/MouseEvent/SpuriousExitEnter/SpuriousExitEnter_3.java fails in jdk6u14 & jdk7
  • 8cc5950: 8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable
  • 4c3491b: 8017175: [TESTBUG] javax/swing/JPopupMenu/4634626/bug4634626.java sometimes failed on mac
  • c0cda1d: 8273026: Slow LoginContext.login() on multi threading application
  • 15fd8a3: 8276102: JDK-8245095 integration reverted JDK-8247980
  • e89b2c0: 8276086: Increase size of metaspace mappings
  • 24cf480: 8276047: G1: refactor G1CardSetArrayLocker
  • ... and 36 more: https://git.openjdk.java.net/jdk/compare/d98b7c25910d38ac644838f59cb41ecd131c87a9...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 29, 2021

@yminqi Pushed as commit cef9db9.

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

@yminqi yminqi deleted the jdk-8276039 branch Oct 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 29, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Yumin,

On 30/10/2021 2:16 am, Yumin Qi wrote:

On Wed, 27 Oct 2021 05:20:37 GMT, Mikael Vidstedt <mikael at openjdk.org> wrote:

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

Remove unnecessary qualifier scope operator in class java_lang_Class

That's funny. I guess `java_lang_Class:` becomes a label and set_init_lock is local to the class so resolution works anyway.

@vidmik @iklam Thanks for review
@dholmes-ora @kimbarrett Thanks for comment

This was not properly reviewed IMO. The "reviews" from Mikael and Ioi
were for the initial one-line change. Then based on the comments you
made much more extensive changes. Those extensive changes were not
actually approved by anyone. I was waiting for the bug and PR titles to
get updated.

David
-----

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 29, 2021

Hi, David
Are you off today? I think the change is simple and also waited for one day --- no objection so do the push.

Thanks for your input.

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 29, 2021

Hi, David Are you off today? I think the change is simple and also waited for one day --- no objection so do the push.

Thanks for your input.

Maybe I should wait until next week to do the push.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 29, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 30/10/2021 7:44 am, Yumin Qi wrote:

On Wed, 27 Oct 2021 23:00:42 GMT, Yumin Qi <minqi at openjdk.org> wrote:

Please review,

I have no idea why this problem has not been encountered in compilation, it is obvious the usage of resolution operator :: is not complete and wrong.

Tests: mach5 tier1, tier4

Thanks
Yumin

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

Remove unnecessary qualifier scope operator in class java_lang_Class

Hi, David
Are you off today? I think the change is simple and also waited for one day --- no objection so do the push.

I was off yesterday (today is Saturday :) )

The change is simple, but the point is you should have at least had your
initial reviewers re-review the second version.

Cheers,
David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
5 participants