8332176: Refactor ClassListParser::parse()

[iklam]

The handling of @ tags in ClassListParser::parse() is quite convoluted. As we expect to add more @ tags in Project Leyden, we should refactor this code to make it more maintainable.

It's easier to review the changes by ignoring the whitespace changes.

• I refactored the parsing to two parts -- parse_at_tags() and parse_class_name_and_attributes()
• I changed the ClassListParser::parse() to no longer return the number of classes loaded -- some @ tags can cause many classes to be loaded as a side effect (e.g., @lambda-form-invoker), so the number returned by ClassListParser::parse() is not accurate. There are other CDS logs that print out the exact number of classes stored in the archive.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8332176: Refactor ClassListParser::parse() (Enhancement - P4)

Reviewers

• Matias Saavedra Silva (@matias9927 - Reviewer)
• Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19225/head:pull/19225
$ git checkout pull/19225

Update a local copy of the PR:
$ git checkout pull/19225
$ git pull https://git.openjdk.org/jdk.git pull/19225/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19225

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19225.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! 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]

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

8332176: Refactor ClassListParser::parse()

Reviewed-by: matsaave, ccheung

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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]

@iklam 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]

Webrevs

• 02: Full - Incremental (33b09389)
• 01: Full - Incremental (43e3eee5)
• 00: Full (11d8e5b6)

[matias9927]

Thanks for the refactor, this is a bit easier to read now! I have a couple of considerations below:

[matias9927]

Unless I'm mistaken, split_tokens_by_whitespace is only called once byparse_at_tags. Do you anticipate that this will be called with multiple "item" lists in the future? If not I think you can leave _indy_items in this method as before.

[iklam]

I changed this function as its name is confusing. If we want to keep the existing code that operates on _indy_items, we have to rename it accordingly, as split_tokens_by_whitespace doesn't mention anything about _indy_items. But that seems to be going the wrong direction, making it more difficult to reuse this function in the future, if such need arises.

I think it's a good idea to reduce coupling in the code. This function splits a line into multiple parts, so we shouldn't make it depend on _indy_items.

[calvinccheung]

Looks good overall. Found an outdated comment.

[matias9927]

Updates look good, thanks!

[iklam]

Thanks @calvinccheung and @matias9927 for the review
/integrate

[openjdk]

Going to push as commit b687aa5.

[openjdk]

@iklam Pushed as commit b687aa5.

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