> On Feb 5, 2024, at 4:31 AM, Magnus Ihse Bursie <i...@openjdk.org> wrote: > > On Mon, 5 Feb 2024 03:21:35 GMT, Kim Barrett <kbarr...@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.
signature.asc
Description: Message signed with OpenPGP
