RFR: 8333211: NMT Reports: replace manual indentation handling with auto indent

Thomas Stuefe stuefe at openjdk.org
Tue Jun 4 07:34:14 UTC 2024


On Tue, 4 Jun 2024 06:34:22 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Hi Thomas, I'm only looking at the auto-indent aspect of this as I'm also working on the stream code and reviewing another change using indentation. To be honest I cannot understand how outputstream's indentation works because to me indentation is always/only something that applies at the start of a new line - hence "auto-indent" would always be the case. I mean "indentation" in the middle of a line is a meaningless concept, so the logic in `indent` has me really bemused.

You are right, the original indentation support is super awkward and unintuitive. But I did not aim to fix that, nor the call sites that use it, that would be a far bigger change. 

I think the contract was that the outputStream keeps track of indentation for you, which is just a number. You can modify it, possibly with an RAII object. But then, you have to call indent() yourself, nothing is done for you.

It is not intuitive. All my patch does is call indent() at the start of a function.

With the autoindent feature, all that is required is to set the indentation level (possibly with streamIndentor), then restrict yourself to a certain kind of usage pattern. A pattern that is very typical anyway, most callers won't notice the restriction.

My thought is that at some point in the future, we may have changed all manual indent()ing code with this new auto-indenting, and then we could rework manual indentation code, possibly removing the indent() function.

> src/hotspot/share/utilities/ostream.hpp line 83:
> 
>> 81:    void dec(int n) { _indentation -= n; };
>> 82:    int  indentation() const    { return _indentation; }
>> 83:    void set_indentation(int i) { _indentation = i;    }
> 
> Why have you deleted these?

They were not needed, nobody uses them. Indentation level is only modified by outputStream itself and it accesses _indentation directly.

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

PR Comment: https://git.openjdk.org/jdk/pull/19461#issuecomment-2146810606
PR Review Comment: https://git.openjdk.org/jdk/pull/19461#discussion_r1625501676


More information about the hotspot-runtime-dev mailing list