RFR: 8330532: Improve line-oriented text parsing in HotSpot [v3]

[John R Rose]

On Tue, 23 Apr 2024 10:08:37 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
>> 
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into 8330532-improve-line-oriented-text-parsing-in-hotspot
>>  - Comments fro @coleenp and @matias9927
>>  - removed more unused code from istream.hpp
>>  - Merged ClassFileParser changes from https://github.com/openjdk/jdk/pull/18669
>>  - Removed gtest cases for features removed in the previous commit
>>  - Reverted xmlstream.cpp/hpp and removed unused functions from inputStream
>>  - fixed builds
>>  - Imported @jrose00 changes https://github.com/openjdk/jdk/pull/18773
>
> src/hotspot/share/utilities/istream.hpp line 98:
> 
>> 96: 
>> 97:   Input* _input;   // where the input comes from or else nullptr
>> 98:   IState _input_state;  // one of {NTR,EOF,ERR}_STATE
> 
> This comment not necessary as the type describes its valid state.

Sometimes redundant comments are helpful.  I think this one is.  YMMV.

> src/hotspot/share/utilities/istream.hpp line 106:
> 
>> 104:   size_t _end;          // offset to end of known current line (else content_end)
>> 105:   size_t _next;         // offset to known start of next line (else =end)
>> 106:   void*  _must_free;    // unless null, a malloc pointer which we must free
> 
> Reading this code, why do we set `_must_free` instead of simply having a method:
> 
> ```c++
> bool must_free() {
>   return _buffer != &_small_buffer;
> }
> 
> 
> and just delete the `_must_free` field.

Good question.  There was a version of the code that accepted a user-supplied buffer, optionally.  In that case, `_must_free` was set false (or to a user-requested value), since it was up to the user whether the user-supplied buffer should be freed.  It could be a static buffer.  If there is no longer such an option in the existing constructors, then this field can be GC-ed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18833#discussion_r1576718468
PR Review Comment: https://git.openjdk.org/jdk/pull/18833#discussion_r1576716061