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

[John McCall via llvm-commits]

On Mon, Apr 23, 2018 at 8:23 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On 23 April 2018 at 16:23, David Blaikie via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> On Mon, Apr 23, 2018 at 4:12 PM John McCall <rjmccall at gmail.com> wrote:
>>
>>> On Mon, Apr 23, 2018 at 6:32 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> On Mon, Apr 23, 2018 at 3:29 PM John McCall via Phabricator <
>>>> reviews at reviews.llvm.org> wrote:
>>>>
>>>>> rjmccall added a comment.
>>>>>
>>>>> In https://reviews.llvm.org/D45766#1076176, @dblaikie wrote:
>>>>>
>>>>> > Is there anything else in the "-w" namespace other than the literal
>>>>> "-w" so
>>>>> >  far?
>>>>>
>>>>>
>>>>> No. This would be novel.
>>>>>
>>>>
>>>> Ah, I see.
>>>>
>>>>
>>>>> > I mean, I could imagine it might make more sense to default these
>>>>> warnings
>>>>> >  off & users can turn them on for non-test code, potentially? So
>>>>> >  "-Wnon-test" might make sense.
>>>>>
>>>>> That's an interesting idea, but it's still not a warning group,
>>>>> because you shouldn't get the self-assign warnings unless `-Wself-assign`
>>>>> is enabled.
>>>>>
>>>>
>>>> You shouldn't?
>>>>
>>>
>>> I wouldn't think so.  Remember that the goal of the option is to be a
>>> single thing that users can add to their unit-test CFLAGS to disable these
>>> noisy-in-tests cases.  So if we add an opt-in/experimental
>>> `-Wunpredictable-foozits` warning, and it has a unit-test carve-out,
>>> passing `-wtest -wno-test` or whatever shouldn't turn on the carved-out
>>> special case of `-Wunpredictable-foozits`.
>>>
>>> It's probably not the worst thing to just use a `-W` spelling anyway;
>>> not everything in that namespace is (e.g. `-Werror`).  It could be
>>> `-Wnoisy-in-tests` and `-Wno-noisy-in-tests`, with a documentation note
>>> that `-Wnoisy-in-tests` is just a cancellation of `-Wno-noisy-in-tests` and
>>> doesn't actually enable any warnings by itself.  We could have the
>>> diagnostic printer add `-Wnoisy-in-tests` to the diagnostic-group
>>> annotation for diagnostics that would be suppressed under
>>> `-Wno-noisy-in-tests`, analogously to how it adds `-Werror` for diagnostics
>>> that have been promoted to an error.
>>>
>>
>> That sort of sounds pretty plausible to me. Poked Richard about his
>> opinion here too.
>>
>
> This is not the only warning group that has the property that only one of
> -Wfoo and -Wno-foo seems useful. There are, for instance, plenty of
> diagnostic groups that make sense to turn on, but not to turn off (eg,
> -Wno-all, -Wno-extra are largely meaningless), and some that make sense to
> turn off, but not to turn on (eg, -Wnon-gcc is only intended to be turned
> off). So I don't think that's as special a property as is being suggested.
> If someone uses -Wnoisy-in-tests and it turns on all warnings that are
> noisy in tests, I think they got what they asked for.
>

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.

This is also not the only warning group for which we want diagnostics to be
> in this group and in some other group or groups. I think the idea to
> include -Wnoisy-in-tests in the diagnostic output is very interesting, but
> as a general feature not as a -Wnoisy-in-tests special case -- for example,
> if I use a binary literal in C++98 mode, I'd like the warning to be tagged
> with [-Wbinary-literal,-Wc++14-extensions,-Wgnu], rather than the current
> [-Wc++14-binary-literal] (an artificial warning group that only exists to
> work around the inability of our infrastructure to properly represent
> warnings that are in multiple groups at once).
>

> As a simple way to get this effect, perhaps we could allow warning groups
> to be tagged as artificial, and for such warning groups, recurse to the
> warning groups including them to find the name to use for diagnostics.
>

That's a really good idea.  I don't think it's a good match for this, but I
completely agree that there's a separate problem of presenting artificial
diagnostic groups to users.

I'm going to quote your other post into this thread in the interest of
making it easier for posterity to read a unified discussion.

I would feel a lot more comfortable adding a `-wtest` flag if we had more
> than one warning that it would control. If there's really only one warning
> out of >600 that qualifies for this treatment, I really don't think we have
> a good argument to introduce a new feature here, and it'll just be dead
> weight we're carrying forever. My inclination is to say that the single
> `-Wno-` flag is sufficient until we actually have multiple warnings that
> this would control, and this is a premature generalization until that
> point. ("We're just about to add these warnings" carries a lot more weight
> for me here than "We have ideas of warnings we might add", but if we're in
> the former situation, there seems to be no harm in waiting.)


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.

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'm also concerned about the deployability and utility of this feature.
> Identifying "test code" is not going to be easy in a lot of build systems.


Really?  In my experience, this is an extremely common thing to split out
in a build system.  That shouldn't be surprising, since test code often has
very different build and/or deployment requirements from the rest of the
project.  That's why Bazel, for example, splits out `cc_test` as a
completely different top-level declaration.  Now perhaps Bazel isn't a
great example for my argument; apparently the underlying machinery is
hard-coded to not allow easy customization of how tests are built, at least
not separately from anything else.  But that's a rather unique limitation
of Bazel and its hard-coded abstractions; a build system written in, say,
cmake would make that a project-wide function which can easily be modified
to pass an extra option.  (I have no idea how easy it would be for Chromium
to do this with gypi; probably very easy if one were willing to change gypi
itself and not so easy if one has to maintain its purity as a metabuild
system.)

Even if successfully deployed, this would mean that refactorings moving
> code between files (eg, out of test code into shared infrastructure code)
> could affect whether clang accepts the program (eg, under `-Werror`), which
> seems pretty undesirable.


The testing idioms that trigger warnings like this are unlikely to be moved
(or be movable) into common infrastructure.  Test infrastructure, maybe,
but I don't know why that would be included in non-test code.  For example,
this self-assign warning can only be abstracted with a macro or a template,
i.e. in code that has to be present in the test's translation unit and
which is going to be useless in a non-test translation unit.


> And likewise, tests that check that (for instance) certain macro
> expansions produce valid code would stop being reliable -- the expansion
> might be valid in test code but not valid in non-test code.


That's a `-Werror`-like argument.  If you're going to what-if on this
level, you can literally never touch the warning code again. :)


> But for me that's a minor concern at worst: if there are users who are
> happy with the tradeoffs here, I'd be OK with us carrying support for them.
> The major concern here is: are there users who would enable this feature?
> (Briefly taking off my Clang hat and putting on my Google hat, I think we
> -- as the people who originally had major problems with the expanded
> `-Wself-assign` warning -- would be very unlikely to ever use `-wtest`
> because of the refactoring issue.)


I think individual projects would absolutely use this feature if we made it
available and told them that it's supposed to be a stable way of avoiding
some false positives in their unit tests.  The warning-by-warning
suppression seems like much more of a compiler-group sort of solution,
suitable for a team like yours or mine that's exhausted by the idea of
making changes to hundreds of different build systems.


> Finally, let's assume that this is successful and we end up with dozens of
> warnings covered by `-wtest`. How confident are we that we're not going to
> want a case-by-case way to turn them back on? That is, how sure are we that
> this isn't just another warning group (albeit one that only really makes
> sense to turn off, not to turn on)? For `-w`, this isn't really a concern,
> because `-w` is very much a "just compile it, I do not care about bugs or
> code quality" flag which doesn't really make sense to only partially deploy.


Well, I think we've already agreed that we'll *also* be providing
warning-by-warning suppress flags for the sake of compiler
maintenance/deployment groups.  I don't know why an individual project
maintainer would ever want that finer-grained control.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180423/2c7c4a80/attachment.html>