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
lebedev.ri on Apr 16 2018, 6:38 AM.Authored by
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?
Uhm, why? -wtest is not the default, so the warning would still have been issued there.
I'm not sure this is a practical direction to pursue - though perhaps
It's likely non-trivial to plumb a flag through most build systems to be
I'm sorry, I don't understand.
If you don't separate between source code and *_test.cpp sources, sure,
What difference is there between passing -wtest and passing -Wno-self-assign-overloaded?
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.
Ok, so this was a non-argument then?
It's a bit hard to talk about all and every spherical build system / project
In llvm's case you would simply add -wtest (or -Wno-self-assign-overloaded)
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.
Let me try to summarize where I think we are.
certainly for field assignments, but we also have (at least?) one example
tests and that we should provide a compiler option to suppress.
rewriting the RHS to avoid the warning) and file-by-file (compiler option)
systems to target it narrowly at their unit-test code; but we also have to
be specific to this warning but instead should be a more generic way of
considering extending some other warnings to cover user-defined operators,
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.
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.