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

[Ioi Lam]

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


-Xlog:all=socket::address=10.1.1.1,port=1234,foldmultiline=true


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