RFR: 8271186: Add UL option to replace newline char [v4]

Ioi Lam iklam at openjdk.java.net
Wed Aug 25 03:50:30 UTC 2021

On Wed, 25 Aug 2021 01:28:16 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

>> src/hotspot/share/logging/logFileOutput.cpp line 204:
>>> 202:       success = LogFileStreamOutput::initialize(pos, errstream);
>>> 203:       *equals_pos = '\0';
>>> 204:       if (!success) {
>> I think the code can be simplified by parsing the value here:
>> bool fold_multilines = ....;
>> LogFileStreamOutput::set_fold_multilines(foldmultilines);
> `foldmultilines` is a parameter which should be applied in initialization. `LogOutput` and extended classes of it look like to have a rule to apply initialization parameter at `initialize()`.
> We can simplify the code as you said if we can apply `foldmultiline` for `LogFileStreamOutput` at `LogFileOutput::initialize()`, but I concern it breaks the semantics for UL implementation.

If I understand correctly, your concern is `_fold_multilines` is a property of `LogFileStreamOutput`, so the parsing of this option should be done inside the `LogFileStreamOutput` class. That way, (in the future) all subclasses of `LogFileStreamOutput` will be able to handle `foldmultiline=true` in their options by deferring to `LogFileStreamOutput::initialize`.

I suppose this would be useful if we have support for sockets in the future, like


However, I don't think your current implementation has the right abstraction -- `FoldMultilinesOptionKey` is now checked both in `LogFileStreamOutput::initialize()` and `LogFileOutput::initialize()`. If we add a new `LogSocketOutput` in the future, we would have to duplicated the code in `LogSocketOutput::initialize()` as well.

I think we should defer the design of such an abstraction until we need it. Ideally, `LogFileOutput` and `LogSocketOutput` should not need to know what options are supported by their superclass, `LogFileStreamOutput`

 For the time being, it's easier to parse all the file output options in `LogFileOutput::initialize()` to keep the code simple.


PR: https://git.openjdk.java.net/jdk/pull/4885

More information about the hotspot-runtime-dev mailing list