RFR: 8325163: Enable -Wpedantic on clang

[Kim Barrett]

> On Feb 5, 2024, at 4:31 AM, Magnus Ihse Bursie <ihse at openjdk.org> wrote:
> 
> On Mon, 5 Feb 2024 03:21:35 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
> 
>>> Inspired by (the later backed-out) [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to enable `-Wpedantic` for clang. This has already found some irregularities in the code, like mistakenly using `#import` instead of `#include`. In this patch, I disable warnings for these individual buggy or badly written files, but I intend to post follow-up issues on the respective teams to have them properly fixed.
>>> 
>>> Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since individual warnings in `-Wpedantic` cannot be disabled. This means that code like this:
>>> 
>>> 
>>> #define DEBUG_ONLY(code) code;
>>> 
>>> DEBUG_ONLY(foo());
>>> 
>>> 
>>> will result in a `; ;`. This breaks the C standard, but is benign, and we use it all over the place. On clang, we can ignore this by `-Wno-extra-semi`, but this is not available on gcc.
>> 
>>> Inspired by (the later backed-out) [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to enable `-Wpedantic` for clang. This has already found some irregularities in the code, like mistakenly using `#import` instead of `#include`. In this patch, I disable warnings for these individual buggy or badly written files, but I intend to post follow-up issues on the respective teams to have them properly fixed.
>> 
>> Rather than first turning on pedantic warnings and then (maybe) going back and perhaps fixing things, I'd really prefer
>> things be done in the other order.  (That's how I handled the recent `-Wparentheses` changes, for example.)
> 
> @kimbarrett
>> Rather than first turning on pedantic warnings and then (maybe) going back and perhaps fixing things, I'd really prefer
> things be done in the other order.
> 
> I hear what you are saying, but I respectfully disagree. If we do things in the order you suggest, the global flag cannot be turned on until all component teams have fixed their source code. That essentially makes the most overworked group putting an effective veto to this change (when your workload is too high, fixing warnings is not on top of your priority.) In contrast, if the global warning can be turned on now, it will have a positive effect on all new and modified code from now on. And the teams can work on their individual warnings, and remove them in their own time.

Without knowing what the actual warnings are that are being triggered and then
suppressed, I can't even begin to evaluate this change. Not all warnings are
good and useful. Sometimes the avoidance effort is really not worth the
benefit. Sometimes there is a future change to the Standard that is already
supported as an extension. Sometimes there is a widely supported extension
that has perhaps just not yet made it into the Standard. Sometimes
platform-specific code uses platform-specific extensions.  And so on.

Enabling -Wpedantic requires an evaluation of the costs and benefits and a
decision that there's a good tradeoff there.  So far, this PR doesn't do that.
Fixing the warnings first, or at least having the relevant bug reports, would
provide that information.

> This is the same method I used for turning on -Wall and -Wextra. Some teams are very eager to fix warnings, and others still have almost all their "DISABLED_WARNINGS" left several years later. (I will not mention any names; you both know who you are ;-)). If I had followed the route you suggest, we would not have -Wall -Wextra on all source code (sans a few, marked files) now.

For -Wparentheses I took the opposite approach and fixed all occurrences
(finding and fixing a couple of bugs in the process) before enabling them.
We've also been approaching -Wconversion from that direction. I think the
enabled but then suppressed warnings has led to an out-of-sight out-of-mind
situation for the suppressed warnings.

I was not particularly happy with adding -Wextra, for example, because I think
some of the warnings it triggers are questionable. You and I went through a
very similar discussion for enabling that option for HotSpot. Right now I
don't even have the information needed for such a discussion.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240205/c2b799b1/signature-0001.asc>