[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 25 19:10:28 PDT 2018
On 23 April 2018 at 20:07, John McCall via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> 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.
>
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.
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 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.)
I really like the idea of a -Wrecommended.
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.)
>
I don't dispute that there exist build systems in which this can be done
easily (though as you note, Bazel is not currently one of them). But there
also exist build systems in which test binaries are just binaries, and test
code lives alongside the rest of the code with no particular distinguishing
features except that it's only linked into tests. My point is that this
feature would not be easily deployed by everyone (and possibly not even by
most), not that it would not be easily deployed by anyone.
> 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.
>
That's a fair point. However, if you're using Clang modules and you're
building them explicitly, the warning flags for your infrastructure code
are the warning flags for the infrastructure target, not those for the user
of that target, even under template instantiation. And likewise for our
behavior in the C++ Modules TS.
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. :)
>
I don't see how that follows; maybe we're talking past each other? I'm
saying:
* You have a library that exports a macro FOO
* You want to test that the expansion of FOO is valid
* So you expand it within your test code
* That compilation succeeds
If you use -Wno-noisy-in-tests for your tests, the above does *not*
guarantee that the macro is actually usable by production code, even if
you're otherwise using the same compiler and flags for your test and
non-test code.
> 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.
>
OK then. If people will actually use this, I don't want to get in the way,
even though I think we'd be missing a chance to solve a more general
problem. I think my preferred spelling would be -wtest, since we want it to
behave as an override to other -W flags, not as a regular last-flag-wins
warning flag, and this also avoids the double-negative.
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.
>
If -wtest forces warnings off in the same way that -w does, then those
fine-grained flags presumably won't work, though. That said, I don't think
this is a big concern, at least not for -wtest, and I can't predict what
other -w flags we'll find we want in the fullness of time.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180425/fcc1d88a/attachment-0001.html>
More information about the cfe-commits
mailing list