RFR: 8341622: Tag-specific disabled default decorators for UnifiedLogging [v6]

[Antón Seoane]

On Mon, 14 Oct 2024 17:36:16 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Antón Seoane has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Replace LogDecorators::Decorator::NoDecorator with LogDecorators::None
>
> src/hotspot/share/logging/logDecorators.hpp line 96:
> 
>> 94: 
>> 95:     const LogSelection& selection() const { return _selection; }
>> 96:   };
> 
> I am uncomfortable with this type erasure. `LogTagType[LogTag::MaxTags + 1 /* = 6 */]` -> `LogTagType*` -> `LogTagType[LogTag::MaxTags /* = 5 */]`. I think this should be rewritten so that `tag_arr` is typed as a `LogTagType[5]`. I think everywhere we have a `const LogTagType parameter[LogTag::MaxTags]` really should have been `const LogTagType (&parameter)[LogTag::MaxTags]` so that this would have been a compile error. 
> 
> My suggestion is to either do the following:
> Suggestion:
> 
>   public:
>     DefaultUndecoratedSelection(LogLevelType level, LogTagType t0, LogTagType t1 = LogTag::__NO_TAG,
>                                 LogTagType t2 = LogTag::__NO_TAG, LogTagType t3 = LogTag::__NO_TAG,
>                                 LogTagType t4 = LogTag::__NO_TAG, LogTagType guard_tag = LogTag::__NO_TAG) : _selection(LogSelection::Invalid) {
>       assert(guard_tag == LogTag::__NO_TAG, "Too many tags specified!");
> 
>       LogTagType tag_arr[LogTag::MaxTags] = { t0, t1, t2, t3, t4 };
> 
>       _selection = LogSelection(tag_arr, false, level);
>     }
> 
>     const LogSelection& selection() const { return _selection; }
>   };
> 
> 
> or maybe even better, do what we do for the `LogTagSet` and have a static helper and a private constructor, so that we can turn all the asserts into compile errors. 
> 
> Something like:
> Suggestion:
> 
>     DefaultUndecoratedSelection(LogLevelType level, LogTagType t0, LogTagType t1, LogTagType t2,
>                                 LogTagType t3, LogTagType t4) : _selection(LogSelection::Invalid) {
>       LogTagType tag_arr[LogTag::MaxTags] = { t0, t1, t2, t3, t4 };
>       _selection = LogSelection(tag_arr, false, level);
>     }
>   public:
> 
>     template <LogLevelType Level, LogTagType T0, LogTagType T1 = LogTag::__NO_TAG, LogTagType T2 = LogTag::__NO_TAG,
>               LogTagType T3 = LogTag::__NO_TAG, LogTagType T4 = LogTag::__NO_TAG,
>               LogTagType GuardTag = LogTag::__NO_TAG>
>     static DefaultUndecoratedSelection make() {
>       STATIC_ASSERT(GuardTag == LogTag::__NO_TAG);
>       return DefaultUndecoratedSelection(Level, T0, T1, T2, T3, T4);
>     }
> 
>     const LogSelection& selection() const { return _selection; }
>   };
> 
> 
> And we can then use `LogDecorators::DefaultUndecoratedSelection::make<level, LOG_TAGS(...)>()` to create them.

Thanks for your comments! I think having some static selection maker that calls a private constructor is the neatest choice here. I'll go for that

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21383#discussion_r1802537723