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

8254733: HotSpot Style Guide should permit using range-based for loops #1488

Closed
wants to merge 1 commit into from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Nov 28, 2020

Please review and vote on this change to the HotSpot Style Guide to
permit the use of range-based for loops in HotSpot code. Range-based
for is a feature added in C++11.

This is a modification of the Style Guide, so rough consensus among
the HotSpot Group members is required to make this change. Only Group
members should vote for approval (via the github PR), though reasoned
objectsions or comments from anyone will be considered. A decision on
this proposal will not be made before Monday 7-Dec-2020 at 12h00 UTC.

Since we're piggybacking on github PRs here, please use the PR review
process to approve (click on Review Changes > Approve), rather than
sending a "vote: yes" email reply that would be normal for a CFV.
Other responses can still use email of course.

/label hotspot


Progress

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

Issue

  • JDK-8254733: HotSpot Style Guide should permit using range-based for loops

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 28, 2020

👋 Welcome back kbarrett! 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 openjdk bot added rfr Pull request is ready for review hotspot hotspot-dev@openjdk.org labels Nov 28, 2020
@openjdk
Copy link

openjdk bot commented Nov 28, 2020

@kimbarrett
The hotspot label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Nov 28, 2020

Webrevs

@kimbarrett
Copy link
Author

kimbarrett commented Nov 28, 2020

The range-based for loop feature is currently of limited utility,
because HotSpot code mostly avoids using the Standard Library and
existing HotSpot code provides relatively little support for the
feature. However, GrowableArray and EnumRange both provide the
necessary begin and end member functions returning iterators. There's
a chicken and egg problem with the latter; there's no reason to
provide support for the feature if it can't be used. By permitting use
of the feature we encourage adding support.

There is at least one use of the feature already present. It was used
in a recent PR; the reviewers noted this but decided to allow it, with
the expectation that the feature would be explicitly permitted soon.

Most uses of the new EnumIterator facility could use and benefit from
the feature.

@openjdk
Copy link

openjdk bot commented Nov 30, 2020

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

8254733: HotSpot Style Guide should permit using range-based for loops

Reviewed-by: dholmes, pliden, jrose, dcubed, iklam, eosterlund, tschatzl, kvn

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

  • a5297bd: 8254939: macOS: unused function 'replicate4_imm'
  • 36c0600: 8257805: Add compiler/blackhole tests to tier1
  • 395b6bd: 8257817: Shenandoah: Don't race with conc-weak-in-progress flag in weak-LRB
  • a265c20: 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl resource
  • e3793e5: 8257514: Fix the issues in jdk.jpackage identified by SpotBugs
  • bbc44f5: 8257186: Size of heap segments is not computed correctlyFix overflow in size computation for heap segments
  • b4b9828: 8254784: javac should reject records with @SafeVarargs applied to varargs record component
  • dcf63f8: 8257788: Class fields could be local in the SunJSSE provider
  • d29c78d: 8257679: Improved unix compatibility layer in Windows build (winenv)
  • 74be819: 8257517: LogCompilation: Add -z to the help messages
  • ... and 155 more: https://git.openjdk.java.net/jdk/compare/1241f800023996d33b39a2b881b2bf7e3b7201c3...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 Pull request is ready to be integrated label Nov 30, 2020
pliden
pliden approved these changes Nov 30, 2020
@vnkozlov
Copy link
Contributor

vnkozlov commented Nov 30, 2020

@kimbarrett

Not related to these changes which are fine.

I looked again on voting description for Style Guide changes. And it references to rough consensus which is not in OpenJDK bylaws :
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.html#L69

I think it is bug (separate from these changes) and should be fixed by using our rule http://openjdk.java.net/bylaws#three-vote-consensus
With Project Lead final vote we will need at least 2 other members votes during 2 weeks review period. I think it is similar to rough consensus.

rose00
rose00 approved these changes Nov 30, 2020
iklam
iklam approved these changes Dec 1, 2020
@kimbarrett
Copy link
Author

kimbarrett commented Dec 1, 2020

@kimbarrett

Not related to these changes which are fine.

I looked again on voting description for Style Guide changes. And it references to rough consensus which is not in OpenJDK bylaws :
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.html#L69

I think it is bug (separate from these changes) and should be fixed by using our rule http://openjdk.java.net/bylaws#three-vote-consensus
With Project Lead final vote we will need at least 2 other members votes during 2 weeks review period. I think it is similar to rough consensus.

I'm going to move this discussion to a new thread in hotspot-dev.

fisk
fisk approved these changes Dec 1, 2020
@kimbarrett
Copy link
Author

kimbarrett commented Dec 8, 2020

There have been no issues raised with the proposed change, and plenty of
support. Thanks for all the reviews.

/integrate

@openjdk openjdk bot closed this Dec 8, 2020
@openjdk 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 Dec 8, 2020
@openjdk
Copy link

openjdk bot commented Dec 8, 2020

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

  • 1d0adbb: 8253644: C2: assert(skeleton_predicate_has_opaque(iff)) failed: unexpected
  • 51ac376: 8256411: Based anonymous classes have a weird end position
  • 0b6b6eb: 8257813: [redo] C2: Filter type in PhiNode::Value() for induction variables of trip-counted integer loops
  • 500ab45: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305
  • 6ff18e3: 8257855: Example SafeVarargsNotApplicableToRecordAccessors breaks test tools/javac/diags/CheckExamples.java
  • cef606f: 8253762: JFR: getField(String) should be able to access subfields
  • 39b8a2e: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
  • c43c224: 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on x86_32
  • 62c7788: 8257211: C2: Enable call devirtualization during post-parse phase
  • 149a02f: 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
  • ... and 167 more: https://git.openjdk.java.net/jdk/compare/1241f800023996d33b39a2b881b2bf7e3b7201c3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 52ab721.

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

@kimbarrett kimbarrett deleted the ranged_for branch Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
9 participants