RFR: 8286117: Remove unnecessary indirection and unused code in UL

[Ioi Lam]

On Thu, 5 May 2022 06:42:59 GMT, Johan Sjölén <duke at openjdk.java.net> wrote:

>> src/hotspot/share/logging/log.hpp line 184:
>> 
>>> 182: 
>>> 183:   static bool is_enabled() {
>>> 184:     return LogTagSetMapping<T0, T1, T2, T3, T4, GuardTag>::tagset().is_level(level);
>> 
>> This removes an abstraction and makes the code more verbose. I don't see the value of doing this, especially when LogImpl is used by macros like `log_info()`, `log_debug()`, etc, which are the primary interface to UL. With this change, people who wonder how `log_info()` works would need to understand more implementation details than before.
>
> Hi @iklam, `LogImpl` is still used in `log_X()` macros! `LogTarget` macro uses `LogTargetImpl`, which this changes. We're not lowering the level of abstraction, we're getting rid of an unneeded one that, to me, made the code even more difficult to understand.
> 
> Let's read the code together, because this was confusing to me when I saw it.
> 
> 
> LogImpl<T0, T1, T2, T3, T4, GuardTag>::is_level(level) ~> // log.hpp:186:LogImplTarget
> LogTagSetMapping<T0, T1, T2, T3, T4>::tagset().is_level(level) ~> // log.hpp:124:LogImpl
> _output_list.is_level(level); // logTagSet.hpp:126:LogTagSet
> 
> 
> When you read `log.hpp:186` you're probably thinking "OK, `LogTargetImpl` *needs* `LogImpl`, which makes sense!" but then you follow the code and LogImpl doesn't provide any extra state for `LogTagSet`. So maybe then you think that `LogImpl` provides some extra state when constructed (nope, empty constructor, LogTagSetMapping constructs LogTagSet for us statically).
> 
> At about that point I made the conclusion that this indirection is unneeded and should be removed :-).
> 
> PS. Accidentally posted this from my *other* Github account, so sorry for the double notification e-mail :-).

OK, that sounds fair. I got mixed up with `LogImpl` and `LogTargetImpl`.

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

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