RFR: 8332176: Refactor ClassListParser::parse()

[Matias Saavedra Silva]

On Tue, 14 May 2024 01:44:02 GMT, Ioi Lam <iklam at openjdk.org> wrote:

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

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

src/hotspot/share/cds/classListParser.cpp line 111:

> 109: 
> 110:     // Each line in the classlist can be one of three forms:
> 111:     if (*_line == '#') {

*_line and _line[0] do the same thing, could you choose one here for consistency? This could also be a switch statement which could be cleaner.

src/hotspot/share/cds/classListParser.cpp line 250:

> 248: }
> 249: 
> 250: void ClassListParser::split_tokens_by_whitespace(int offset, GrowableArray<const char*>* items) {

Unless I'm mistaken, `split_tokens_by_whitespace` is only called once by`parse_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.

-------------

PR Review: https://git.openjdk.org/jdk/pull/19225#pullrequestreview-2056147274
PR Review Comment: https://git.openjdk.org/jdk/pull/19225#discussion_r1600490241
PR Review Comment: https://git.openjdk.org/jdk/pull/19225#discussion_r1600508159