[Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.
Needs ReviewPublic

Authored by lebedev.ri on Apr 16 2018, 6:38 AM.

Details

Summary

Credit for the idea goes to @rjmccall.

As tests show, only the overloaded non-field assign is silenced by it.

Testing: $ ninja check-clang-sema check-clang-semacxx check-clang

Diff Detail

Repository
rC Clang
lebedev.ri created this revision.Apr 16 2018, 6:38 AM
  • Don't mis-spell the name of the flag.

FIXME: So do we want it to be -Wtests, -Wtest, or we really want it to be -wtest ?

lebedev.ri retitled this revision from [Sema] Add -Wtest global flag that silences -Wself-assign for overloaded operators. to [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators..
lebedev.ri edited the summary of this revision. (Show Details)

Actually make it -wtest (all lowercase).

I don't understand the spelling of this option. You spell it -wtest (because rjmccall suggested that spelling); but for my money it should be spelled -Wno-self-assign-nonfield.
Also, if you go this route, you miss the true positive reported by someone on the other patch, where the self-assignment involved a local variable named color.
Sorry I can't find the example right now. You'd have to trawl through all the patches opened on this subject in the past couple of weeks.

I don't understand the spelling of this option. You spell it -wtest (because rjmccall suggested that spelling); but for my money it should be spelled -Wno-self-assign-nonfield.

You did read the documentation change, right?

Also, if you go this route, you miss the true positive reported by someone on the other patch, where the self-assignment involved a local variable named color.

Uhm, why? -wtest is not the default, so the warning would still have been issued there.

Sorry I can't find the example right now. You'd have to trawl through all the patches opened on this subject in the past couple of weeks.

I'm not sure this is a practical direction to pursue - though perhaps
others disagree.

It's likely non-trivial to plumb a flag through most build systems to be
applied only to test code (& likely would suppress the warning in headers
only included in test code - so for example, in a header-only library if
the user ran their tests they wouldn't see the warning, but when the users
code was used in some other production code it might warn (& possibly break
the build if -Werror is in use))

I'm not sure this is a practical direction to pursue - though perhaps
others disagree.

It's likely non-trivial to plumb a flag through most build systems to be
applied only to test code

I'm sorry, I don't understand.

If you don't separate between source code and *_test.cpp sources, sure,
you will loose the warning coverage either way.

What difference is there between passing -wtest and passing -Wno-self-assign-overloaded?
Just pass it alongside with the googletest include paths so to speak.

(& likely would suppress the warning in headers
only included in test code - so for example, in a header-only library if
the user ran their tests they wouldn't see the warning, but when the users
code was used in some other production code it might warn (& possibly break
the build if -Werror is in use))

Could you please explain how it is not an issue if this would be a -Wno-self-assign-overloaded?

Uuuh, the fact that phab posts the top-postings, but silently ignores inline replies is annoying.

lebedev.ri added a comment.

In https://reviews.llvm.org/D45685#1069040, @dblaikie wrote:

> I'm not sure this is a practical direction to pursue - though perhaps
> others disagree.

> It's likely non-trivial to plumb a flag through most build systems to be
> applied only to test code

I'm sorry, I don't understand.

If you don't separate between source code and *_test.cpp sources, sure,
you will loose the warning coverage either way.

What difference is there between passing -wtest and passing
-Wno-self-assign-overloaded?

Not much, if any.

Ok, so this was a non-argument then?

Just pass it alongside with the googletest include paths so to speak.

But build systems don't necessarily expose that kind of ability. (For
example, googletest (not the only kind of test suite/code) is checked into
the LLVM codebase like another library - so depending on it is just another
library dependency, not a special "test" library dependency).

It's a bit hard to talk about all and every spherical build system / project
in the vacuum, because there is an infinite number of possibilities.
Of course some build systems are horribly designed, and it will be hard to
do that there. But I sure hope that isn't the case in most of the cases.
It might be more productive to talk about a few good known realities.

In llvm's case you would simply add -wtest (or -Wno-self-assign-overloaded)
in here https://github.com/llvm-mirror/llvm/blob/2a6cf85828509e89e18739e5f4b9a958820d66d4/cmake/modules/AddLLVM.cmake#L1079-L1085
and around here https://github.com/llvm-mirror/libcxx/blob/73e00f8321b13559b3c41f6656686d980fe92fbe/utils/libcxx/test/config.py#L914
I'd say that is rather trivial.

+1 to just adding a dedicated Wno flag for the new warning instead of
coming up with this new spelling.

The goal of having a unified option is that you can uniformly suppress warnings that don't always make sense in unit tests. It's future-proofing against the addition of other warnings (probably C++ warnings) that might not make sense for unit tests, like extending the x &= x warning (which is not in -Wself-assign) to user-defined operators. You don't think you would be able to take advantage of that? Because -Wno-self-assign-overloaded-nonfield is rather, uh, pretty precisely targeted for exactly that use case; I can't imagine why someone would use -Wself-assign-overloaded-nofield *positively*, or even *negatively* for any purpose besides suppressing a unit-test problem.

If you can't reasonably just add this to unit tests, of course you can just add it globally, but that's just as true for -wtestas it would be for -Wno-self-assign-overloaded-nonfield, and the former seems more self-descriptive of the problem when it appears in a general CFLAGS line: you don't have a reasonable way of suppressing it for just unit tests so you have to suppress it globally.

I'm sorry, that warning *is* in self-assign, although I'm not sure it should be.

Let me try to summarize where I think we are.

  1. I think it’s now generally agreed that this is a useful warning —

certainly for field assignments, but we also have (at least?) one example
with a local variable.

  1. It’s also generally agreed that this warning has a problem with unit

tests and that we should provide a compiler option to suppress.

  1. There isn’t really a middle-point between case-by-case suppression (by

rewriting the RHS to avoid the warning) and file-by-file (compiler option)
suppression. We don’t know how to distinguish the unit-test false positives
from the local-variable true positive.

  1. Whatever the file-by-file suppression is, it would be best for build

systems to target it narrowly at their unit-test code; but we also have to
acknowledge that that’s not always straightforward, and so the suppression
should be narrowly-targeted on the compiler side as well, just in case the
suppression does have to be more global.

  1. Roman and I are suggesting that the file-by-file suppression should not

be specific to this warning but instead should be a more generic way of
disabling warnings that are known to be problematic in unit tests. We could
then recommend that option to project owners as a single mitigation rather
than forcing them to maintain a growing list of test-directed warning
suppressions.

  1. Using a more general mechanism seems advisable because we are already

considering extending some other warnings to cover user-defined operators,
and all such warnings have the potential of running afoul of unit tests.
(This is something that will require careful thought because user-defined
operators need not have their conventional meaning. However, any
semantics-based suppression will almost certainly have different
characteristics from a test-directed suppression. For example,
test-directed suppression for self-assign need not suppress warnings about
fields, whereas a semantics-based suppression should.)

John.

Thanks for the summary, John. To confirm, I found two examples of bugs involving local variables, as well as the field-based examples. And, indeed, all of the false positives were in unit tests.

@brooksmoses The simple -Wno-self-assign-overloaded variant (D45766) has landed, so the issue should be resolved?

Not sure what to do with this differential, should i abandon it until there is more interest for such functionality?

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

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

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.

include/clang/Basic/Diagnostic.td
102–104

This does not seem like it should ever be necessary.