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

8253397: Ensure LogTag types are sorted #274

Closed
wants to merge 2 commits into from

Conversation

cl4es
Copy link
Member

@cl4es cl4es commented Sep 21, 2020

  • Sort LogTag type enum alphabetically
  • Assert that the tags are sorted instead of sorting

Progress

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

Issue

Reviewers

Download

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

@cl4es cl4es changed the title Ensure LogTag types are sorted 8253397: Ensure LogTag types are sorted Sep 21, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 21, 2020

👋 Welcome back redestad! 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 bot commented Sep 21, 2020

@cl4es 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 (add|remove) "label" command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 21, 2020
@cl4es cl4es marked this pull request as ready for review September 21, 2020 05:27
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 21, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 21, 2020

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Seems fine.

Thanks

@openjdk
Copy link

openjdk bot commented Sep 21, 2020

@cl4es This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253397: Ensure LogTag types are sorted

Reviewed-by: dholmes, kbarrett, tschatzl
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the master branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate b8ea80af33ecc9d4ccfa3c3dc366e1a88c3607b4.

➡️ 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 Sep 21, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 21, 2020

Mailing list message from Kim Barrett on hotspot-runtime-dev:

I think having the LOG_TAG_LIST in sorted order is good.

But I think the checking can be improved. I think it can be done at
compile-time using constexpr, completely eliminating the runtime execution
and data. I'm currently prototyping; I'll let you know what I come up with.

Make LogTag::_name constexpr (it should at least be const, and it's a
bug that it isn't). This will require moving the definition into the
header, but that's okay, it's small.

(I think the sorting could be done at compile-time too, unless that requires
constexpr features from C++17/20; I don't recall off-hand. But having the
list start sorted removes the need for that complexity, and may be better
for usability anyway. Grouping tags by usage doesn't entirely work, since
tags may be used in multiple ways.)

@mlbridge
Copy link

mlbridge bot commented Sep 23, 2020

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Sep 21, 2020, at 6:35 AM, Kim Barrett <kim.barrett at oracle.com> wrote:

I think having the LOG_TAG_LIST in sorted order is good.

But I think the checking can be improved. I think it can be done at
compile-time using constexpr, completely eliminating the runtime execution
and data. I'm currently prototyping; I'll let you know what I come up with.

It's not hard to make the is_sorted check constexpr, but doesn't provide a
real benefit compared to the proposed conditional compilation approach;
neither has any additional code or computation in a product build.

A pre-existing thing that should be fixed though:
src/hotspot/share/logging/logTag.hpp
231 static const char* _name[];

That should be
231 static const char* const _name[];

And similarly in the .cpp file.

Looks good with the above change.

@cl4es
Copy link
Member Author

cl4es commented Sep 23, 2020

/integrate

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

openjdk bot commented Sep 23, 2020

@cl4es Pushed as commit 5f1d612.

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

@cl4es cl4es deleted the jdk-8253397 branch September 23, 2020 18:48
@dholmes-ora
Copy link
Member

@cl4es Please do not force push updates to a PR. There is no way for reviewers to see what, if anything, differs between the final commit and the commit that they reviewed. If you want to merge with mainline/master then just merge and push the new commit to your personal fork. The commits will be flattened as part of the actual integration. Thanks.

@tschatzl
Copy link
Contributor

tschatzl commented Sep 24, 2020

@dholmes-ora: fwiw, there is: click on the actual commit link (e.g. 19d37e7 in this case where it says " cl4es added 2 commits 4 days ago ") and you'll get a diff in github. There is an enhancement/bug open on skara for better support of diffs for force pushes.

@cl4es
Copy link
Member Author

cl4es commented Sep 24, 2020

@dholmes-ora once I ended up in a state where force-push seemed like the only option it was too late. I assure you there were no changes from the reviewed changeset, and since there were no inline comments little I figured little was lost.

I believe a git rebase was the cause of this, and will use git merge in my workflow instead. I got into the habit of continually rebasing rather than merging in mercurial in order to avoid merge commits, but here those will be squashed anyhow. I will refrain from rebasing after opening a PR from now on.

@dholmes-ora
Copy link
Member

@dholmes-ora: fwiw, there is: click on the actual commit link (e.g. 19d37e7 in this case where it says " cl4es added 2 commits 4 days ago ") and you'll get a diff in github. There is an enhancement/bug open on skara for better support of diffs for force pushes.

That is a diff for one of the commits that was force-pushed over the original. I can see diffs both of those commits but what I cannot see is how those commits differ to the original one that was overwritten. Further if I click on "Show changes since your last review" I get an error page:

"We went looking everywhere, but couldn’t find those commits.
Sometimes commits can disappear after a force-push. Head back to the latest changes here."

We don't need better support for forced pushes IMO we just need to not do them. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants