<div dir="ltr">On Wed, Apr 25, 2018 at 7:10 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 23 April 2018 at 20:07, John McCall via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></div></span><div>The issue there is that -Wnoisy-in-tests is likely to be useful as a cancellation of -Wno-noisy-in-tests, which (as David suggests) might reasonably be set by default by a build system.  That's completely defeated if it potentially enables a bunch of unwanted warnings.</div></div></div></div></blockquote><div><br></div><div>Yes, that's fair, but it also applies to some of the other groups I mentioned. Eg, how can a project undo a build system default of -Wall -Wextra, without turning off enabled-by-default warnings that happen to also be in those groups?</div><div><br></div><div>If we want to address the broader problem here, I think we need a more general model for mapping from a sequence of warning flags to a set of enabled/disabled choices for warnings. We could let each warning specify an arbitrary positive unate function over warning flags that determines whether it's enabled, but I expect that'd be a bit too complex for comprehensibility. So I'm not sure what the best way forward is here, but I'd prefer that it's not a one-off special thing with its own unique semantics, at least if it's in the -W namespace. Maybe having a layer of "-w" flags that override the "-W" flags really is the best model.</div></div></div></div></blockquote><div><br></div><div>I believe I know a better model (although perhaps that ship has sailed). Simply keep track of all "-W" flags individually by name, and make "-Wno-foo" <i>merely</i> cancel out a prior "-Wfoo" whenever "foo" is the name of a group (as opposed to an individual diagnostic). Thus "-Wall -Wmove -Wno-all" would be equivalent to simply "-Wmove"; whereas "-Wall -Wno-pessimizing-move" would keep its current behavior (because "no-pessimizing-move" is the name of an individual diagnostic); and "-Wall -Wno-move" would be equivalent to "-Wall" (oops) unless we do something even more clever.</div><div><br></div><div>Green Hills' compiler driver (which used EDG's front-end and so I think most EDG compilers would do the same thing?) completely solves the problem by using a diagnostic model that is different from GCC's model. In EDG's model,</div><div>- each diagnostic has a unique permanent identifying number</div><div>- each diagnostic has a default level, chosen by the compiler-writer, from the set {none, remark, warning, error, fatal}<br></div><div>- all diagnostic options are of the form "--diag-remark=1234", "--diag-error=42", etc.</div><div>- there is a special option "--diag-default=1234" to clear out any prior setting for diagnostic 1234</div><div>- Diagnostics with default level "fatal" are not allowed to be set to any other level.</div><div>- Orthogonally to all of the above, there is a driver option to control the minimum level that a diagnostic must be at, in order to be printed: --errors-only, --warnings-only (the default), --remarks, or --everything (which prints even the diagnostics whose level is "none").</div><div><br></div><div>So for example if diagnostic 42 is a remark and diagnostic 43 is a warning, then</div><div>- "cc86" will print "Warning 43: blah."</div><div></div><div>- "cc86 --errors-only" will print nothing.</div><div>- "cc86 --diag-warning=42" will print "Warning 42: blah. Warning 43: blah."<br></div><div>- "cc86 --diag-error=42" will print "Error 42: blah. Warning 43: blah."</div><div>- "cc86 --diag-remark=43" will print nothing.<br></div><div><div>- "cc86 --diag-remark=43 --remarks" will print "Remark 42: blah. Remark 43: blah."</div></div><div><div>- "cc86 --remarks" will print "Remark 42: blah. Warning 43: blah."</div></div><div>And, most relevantly to this discussion,</div><div>- "cc86 --diag-error=42 --diag-default=42" will print "Warning 43: blah."</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div>This is fair.  This is certainly an early generalization.  I only think it might not be premature because I think it gives us a very clear message for projects: add `-wtest` when building unit tests and (maybe) unit test infrastructure, and the compiler will try to avoid warning about idioms that don't make sense in normal code but might be sensible in unit tests.  If we wait — even if we're only waiting for a second warning — we're jerking project maintainers around.</div></div></div></blockquote></div></div></div></blockquote><div><br></div><div>You mean because a project maintainer will upgrade their Clang, and then it'll start warning about self-assignments in their test code, and they'll have to shut it up by passing -Wno-self-assign, and then in the next Clang release they'll have to change the driver option to -wtest, or something?  I'm not sure I understand.  Surely what we <i>hope</i> will happen is one of these two scenarios instead:</div><div>(1) They upgrade Clang and it doesn't start warning about self-assignments because we decided the signal-to-noise ratio of that particular heuristic was too low to justify including it in -Wall. They don't have to do anything. Everything's great.</div><div>(2) They upgrade Clang and it does start warning about self-assignments, but the warning includes a fixit to throw parens around the second `x`, or something. They do so, and the warning goes away for that line of code forever. Everything's great.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div></div><div>In general, I think we should be doing a lot more to provide clearer "best practices" recommendations to our users about warnings.  We may not be willing to add new warnings to the defaults or `-Wall`, but we could certainly add a curated `-Wrecommended` (just one!) with an understanding that the set of warnings in that group will grow over time, and with explicit documentation that projects should not combine it with `-Werror` in builds where they don't themselves specify an exact compiler version.  If users didn't like our recommendations, they could of course continue to tweak them with the finer-grained controls, or just ignore them entirely.</div></div></div></blockquote><div><br></div><div>I agree with the philosophy here, but I am as yet unconvinced that using different warning flags for test and non-test code is really a best practice. Rather, I think best practice would be to use the exact same compiler configuration for test and production code, so that you're actually testing the thing you intend to deploy.</div><div><br></div><div>For this self-assign warning, if the signal-to-noise ratio is actually high enough to justify having it at all, I think we should have an idiomatic source-level construct to express that the self-assignment is intentional, and we should suggest that construct in the diagnostic. (Currently, "x = *&x;" appears to be the suggestion, but I think that's overly cumbersome; "(x) = (x);" or similar would seem more elegant.)</div></div></div></div></blockquote><div><br></div><div>+1. FWIW, I would expect either "x = (x);" or "(x = x);" to be the appropriate spelling; but as long as the diagnostic includes a fixit, I don't have to remember the spelling so I won't care.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I really like the idea of a -Wrecommended.<br></div></div></div></div></blockquote><div><br></div><div>To me, this "-Wrecommended" sounds like what we used to call "-W -Wall -Wextra", which is what we used to call "-Wall", which is what we used to call "just invoking the compiler."  I really would like to avoid "grade inflation" here. Can't we just figure out whether a warning is useful or not, and if it's useful, enable it, and if not, don't?  In this specific case, the signal-to-noise ratio is low, so we should leave it off by default; or maybe have a discussion about whether it goes in -Wextra (but -Wextra is kind of gross anyway).</div><div><br></div><div>my $.02,</div><div>–Arthur</div></div></div></div>