[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

Wed Apr 25 22:37:22 PDT 2018

On Wed, Apr 25, 2018 at 7:10 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On 23 April 2018 at 20:07, John McCall via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>>
>> 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.
>>
>
> 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?
>
> 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.
>

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" *merely* 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.

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,
- each diagnostic has a unique permanent identifying number
- each diagnostic has a default level, chosen by the compiler-writer, from
the set {none, remark, warning, error, fatal}
- all diagnostic options are of the form "--diag-remark=1234",
"--diag-error=42", etc.
- there is a special option "--diag-default=1234" to clear out any prior
setting for diagnostic 1234
- Diagnostics with default level "fatal" are not allowed to be set to any
other level.
- 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").

So for example if diagnostic 42 is a remark and diagnostic 43 is a warning,
then
- "cc86" will print "Warning 43: blah."
- "cc86 --errors-only" will print nothing.
- "cc86 --diag-warning=42" will print "Warning 42: blah. Warning 43: blah."
- "cc86 --diag-error=42" will print "Error 42: blah. Warning 43: blah."
- "cc86 --diag-remark=43" will print nothing.
- "cc86 --diag-remark=43 --remarks" will print "Remark 42: blah. Remark 43:
blah."
- "cc86 --remarks" will print "Remark 42: blah. Warning 43: blah."
And, most relevantly to this discussion,
- "cc86 --diag-error=42 --diag-default=42" will print "Warning 43: blah."

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.
>>
>
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 *hope* will happen is one of
these two scenarios instead:
(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.
(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.

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.
>>
>
> 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.
>
> 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.)
>

+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.

I really like the idea of a -Wrecommended.
>

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).

my $.02,
–Arthur
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180425/63f0ce78/attachment.html>