Hello,

> To make it easier to review this PR, each commit that is prefixed with "FIX:" 
> has the command/method I've used to verify that the output of prints is not 
> changed/broken.

The goal of this PR is to refactor the usage of streamIndentor and 
StreamAutoIndentor and combine them into a single StreamIndentor class to make 
it easier to apply indentation. 
[JDK-8354362](https://bugs.openjdk.org/browse/JDK-8354362) introduced a partial 
solution for the refactor I propose, by being able to add indentation with a 
StreamAutoIndentor.

To use indentation, you currently need to do two separate things: 1) increment 
indentation and 2) apply indentation when printing. The indentation level can 
be incremented by using one of: streamIndentor, inc() or inc(int n), and 
automatic indentation can be enabled by using one of: StreamAutoIndentor, 
indent(), cr_indent().

I propose to make it easier to use and apply indentation by combining 
streamIndentor and StreamAutoIndentor into a new class called StreamIndentor. 
StreamIndentor has the functionality of both streamIndentor and 
StreamAutoIndentor, that is, enabling automatic indentation and also applying 
an optional amount of indentation. This means you only need to do one thing to 
make sure that indentation is incremented and applied, making it easier to use 
indentation.

There are currently four (4) ways that indentation is applied in HotSpot:

1) The new method of enabling automatic indentation and applying indentation 
simultaneously (partially implemented already).
Only in GC printing code via CollectedHeap.

2) Initially use a StreamAutoIndentor, then use streamIndentor to temporarily 
increment indentation. Indentation is automatically applied when printing.
nmt/memReporter uses this principle, by having a StreamAutoIndentor as a member 
variable and applying indentation via streamIndentor when needed.

3) Use a streamIndentor and manually call indent() (or cr_indent()).
Commonly used pattern. No need to manually apply indentation if automatically 
applied.

4) Increment/decrement indentation (using inc() and/or dec()) and manually call 
indent().
Only C1's CFGPrinterOutput does this.

I leave the fourth case alone as it is fairly self-contained and requires a 
more extensive re-write, appropriate for a follow-up patch if that's what we 
want.

In the third case, there is a possibility that there are cases where applying 
indentation automatically is undesirable. However, when refactoring and looking 
at the code I have not found any place where this is the case. All cases when 
using a streamIndentor and applying indentation manually using 
indent()/cr_indent() are able to be replaced by the new StreamIndentor without 
changing the output.

After the changes in this PR there are only two ways that indentation is 
applied:
1) The new method of enabling automatic indentation and applying indentation 
simultaneously using StreamIndentor.
2) Increment/decrement indentation (using inc() and/or dec()) and manually call 
indent().
Only C1's CFGPrinterOutput does this.

Some practical changes:
 * `outputStream::cr_indent()` has been removed since it is no longer 
used/needed.
 * `outputStream::set_autoindent()` is now private and StreamIndentor is a 
friend of outputStream to access this.
 * Made the indentation argument of StreamIndentor explicit, instead of the 
default of 2 which streamIndentor had. Sure, there are many cases which use an 
indentation of two, but I don't feel it's necessary to have this default only 
because it is common. If anything, making the argument explicit makes it easier 
to understand the code, in my opinion.
 * Renamed all uses of a StreamIndentor to `si[level]`.

Testing:
* GHA, Oracle's tier 1-4
* Manual inspection of printed content. See the commit message of commits 
prefixed with "FIX:".

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

Commit messages:
 - Remove cr_indent()
 - Adapt gtest
 - Make outputStream::set_autoindent private
 - Rename to StreamIndentor si[level]
 - Combine streamIndentor and StreamAutoIndentor
 - FIX: memReporter
 - FIX: compilationMemoryStatistic.cpp
 - FIX: Trivial changes in compilationMemoryStatistic.cpp
 - FIX: metaspaceStatistics.cpp
 - FIX: printCLDMetaspaceInfoClosure.cpp
 - ... and 6 more: https://git.openjdk.org/jdk/compare/b41e0b17...a0c122ca

Changes: https://git.openjdk.org/jdk/pull/24917/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24917&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8355692
  Stats: 195 lines in 34 files changed: 27 ins; 50 del; 118 mod
  Patch: https://git.openjdk.org/jdk/pull/24917.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24917/head:pull/24917

PR: https://git.openjdk.org/jdk/pull/24917

Reply via email to