RFR(S): 8176298: Log tags in -Xlog:help not sorted

[Marcus Larsson]

Hi Thomas,

Thanks for reviewing!


On 2018-02-15 18:20, Thomas Stüfe wrote:
> Hi Marcus,
>
> I am confused, are the logtags in logTag.hpp not sorted already? So, 
> the name array should be already sorted?

You're right. They're currently sorted in logTag.hpp, but there's also 
the extension point LOG_TAG_LIST_EXT to extend the list with additional 
tags. Using this causes the list to be unsorted, since the tags are just 
appended to the end.

>
> Beside that, some small nits:
>
> - In LogConfiguration::print_command_line_help(), could we not use the 
> stream for the whole function? Or, better yet, make the function take 
> an outputStream* instead of a FILE*? That would fit nicely with 
> LogConfiguration::describe_available().

Yes, this was actually something I wanted to do, but initially thought 
it was too unrelated to the issue at hand. Now that you brought it up as 
well, I'll include that as part of this fix. :)

>
> - LogTag::list_tags(): I am a tiny bit concerned about using up stack 
> space here, especially if this is used during error reporting. The 
> array is - if I am not mistaken - sizeof(enum) * ~120, so 400-500 
> bytes. Maybe use malloc instead?

Hmm. I was hoping that this would be OK, but I see your concern about 
the error reporter. I found a way to do it with static storage instead, 
sorting it only once during static initialization. See if you like it, 
otherwise we can just do malloc I guess.

Updated webrev:
http://cr.openjdk.java.net/~mlarsson/8176298/webrev.01

Incremental:
http://cr.openjdk.java.net/~mlarsson/8176298/webrev.00-01

Thanks,
Marcus

>
> Otherwise, this looks fine.
>
> Best Regards, Thomas
>
> On Thu, Feb 15, 2018 at 9:12 AM, Marcus Larsson 
> <marcus.larsson at oracle.com <mailto:marcus.larsson at oracle.com>> wrote:
>
>     Hi,
>
>     Please review the following patch to keep log tags sorted when listed.
>
>     Issue:
>     https://bugs.openjdk.java.net/browse/JDK-8176298
>     <https://bugs.openjdk.java.net/browse/JDK-8176298>
>
>     Webrev:
>     http://cr.openjdk.java.net/~mlarsson/8176298/webrev.00
>     <http://cr.openjdk.java.net/%7Emlarsson/8176298/webrev.00>
>
>     Verified with new unit test.
>
>     Thanks,
>     Marcus
>
>