[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.




Comment at: include/clang/Basic/Diagnostic.td:102-104
+class ShowInTests {
+  bit HideInTests = 0;
+}

This does not seem like it should ever be necessary.


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@brooksmoses The simple `-Wno-self-assign-overloaded` variant 
(https://reviews.llvm.org/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?


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.

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

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

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

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

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

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


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[John McCall via cfe-commits]

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.

2. It’s also generally agreed that this warning has a problem with unit
tests and that we should provide a compiler option to suppress.

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

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

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

6. 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.
On Wed, Apr 18, 2018 at 13:54 John McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> rjmccall added a comment.
>
> I'm sorry, that warning *is* in self-assign, although I'm not sure it
> should be.
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D45685
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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 `-wtest`as 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.


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Nico Weber via cfe-commits]

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

On Mon, Apr 16, 2018, 3:41 PM Roman Lebedev via Phabricator via cfe-commits
 wrote:

> lebedev.ri added a comment.
>
> 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.
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D45685
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[David Blaikie via cfe-commits]

On Mon, Apr 16, 2018 at 12:08 PM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:

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


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


>
> > (& 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`?
>

It isn't any different - though it sounds like "-wtest" is being proposed
as a way that test code could specifically be corrected without impacting
the rest of the code. I'm not sure that's true/viable/accessible to most
developers, so the justification seems a bit infeasible - likely naming it
-Wno-self-assign-overloaded would at least name it how it'll be used, to
turn the warning off across a whole codebase because the false positive
rate across the whole codebase is unacceptable for a given user.


>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D45685
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

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`?
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`?


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[David Blaikie via cfe-commits]

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

On Mon, Apr 16, 2018 at 8:10 AM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:

> lebedev.ri updated this revision to Diff 142636.
> 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.
> lebedev.ri added a comment.
>
> Actually make it `-wtest` (all lowercase).
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D45685
>
> Files:
>   docs/ReleaseNotes.rst
>   docs/UsersManual.rst
>   include/clang/AST/ASTDiagnostic.h
>   include/clang/AST/CommentDiagnostic.h
>   include/clang/Basic/Diagnostic.h
>   include/clang/Basic/Diagnostic.td
>   include/clang/Basic/DiagnosticIDs.h
>   include/clang/Basic/DiagnosticOptions.def
>   include/clang/Basic/DiagnosticSemaKinds.td
>   include/clang/CrossTU/CrossTUDiagnostic.h
>   include/clang/Driver/DriverDiagnostic.h
>   include/clang/Driver/Options.td
>   include/clang/Frontend/FrontendDiagnostic.h
>   include/clang/Lex/LexDiagnostic.h
>   include/clang/Parse/ParseDiagnostic.h
>   include/clang/Sema/SemaDiagnostic.h
>   include/clang/Serialization/SerializationDiagnostic.h
>   include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
>   lib/Basic/DiagnosticIDs.cpp
>   lib/Basic/Warnings.cpp
>   lib/Driver/Driver.cpp
>   lib/Frontend/CompilerInvocation.cpp
>   lib/Sema/SemaExpr.cpp
>   test/Sema/warn-self-assign-builtin-warn-test.c
>   test/SemaCXX/warn-self-assign-builtin-warn-test.cpp
>   test/SemaCXX/warn-self-assign-field-builtin-warn-test.cpp
>   test/SemaCXX/warn-self-assign-field-overloaded-warn-test.cpp
>   test/SemaCXX/warn-self-assign-overloaded-warn-test.cpp
>   tools/diagtool/DiagnosticNames.cpp
>   utils/TableGen/ClangDiagnosticsEmitter.cpp
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: lebedev.ri.
dblaikie added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45685#1068981, @Quuxplusone wrote:

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




Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 142636.
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.
lebedev.ri added a comment.

Actually make it `-wtest` (all lowercase).


Repository:
  rC Clang

https://reviews.llvm.org/D45685

Files:
  docs/ReleaseNotes.rst
  docs/UsersManual.rst
  include/clang/AST/ASTDiagnostic.h
  include/clang/AST/CommentDiagnostic.h
  include/clang/Basic/Diagnostic.h
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticOptions.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/Driver/DriverDiagnostic.h
  include/clang/Driver/Options.td
  include/clang/Frontend/FrontendDiagnostic.h
  include/clang/Lex/LexDiagnostic.h
  include/clang/Parse/ParseDiagnostic.h
  include/clang/Sema/SemaDiagnostic.h
  include/clang/Serialization/SerializationDiagnostic.h
  include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
  lib/Basic/DiagnosticIDs.cpp
  lib/Basic/Warnings.cpp
  lib/Driver/Driver.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaExpr.cpp
  test/Sema/warn-self-assign-builtin-warn-test.c
  test/SemaCXX/warn-self-assign-builtin-warn-test.cpp
  test/SemaCXX/warn-self-assign-field-builtin-warn-test.cpp
  test/SemaCXX/warn-self-assign-field-overloaded-warn-test.cpp
  test/SemaCXX/warn-self-assign-overloaded-warn-test.cpp
  tools/diagtool/DiagnosticNames.cpp
  utils/TableGen/ClangDiagnosticsEmitter.cpp

Index: utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -552,6 +552,11 @@
 else
   OS << ", false";
 
+if (R.getValueAsBit("HideInTests"))
+  OS << ", true";
+else
+  OS << ", false";
+
 // Category number.
 OS << ", " << CategoryIDs.getID(getDiagnosticCategory(, DGParentMap));
 OS << ")\n";
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -28,9 +28,9 @@
 // FIXME: Is it worth having two tables, especially when this one can get
 // out of sync easily?
 static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
-#define DIAG(ENUM,CLASS,DEFAULT_MAPPING,DESC,GROUP,   \
- SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY)\
-  { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
+#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR,  \
+ SHOWINSYSHEADER, HIDEINTESTS, CATEGORY)   \
+  {#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
 #include "clang/Basic/DiagnosticCommonKinds.inc"
 #include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
Index: test/SemaCXX/warn-self-assign-overloaded-warn-test.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-overloaded-warn-test.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -wtest -verify -DSILENCE %s
+
+struct S {};
+
+void f() {
+  S a;
+#ifndef SILENCE
+  a = a; // expected-warning{{explicitly assigning}}
+#else
+  // expected-no-diagnostics
+  a = a;
+#endif
+}
Index: test/SemaCXX/warn-self-assign-field-overloaded-warn-test.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-field-overloaded-warn-test.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -wtest -verify %s
+
+struct S {};
+
+struct C {
+  S a;
+
+  void f() {
+#ifndef SILENCE
+a = a; // expected-warning{{assigning field to itself}}
+#else
+// expected-no-diagnostics
+a = a;
+#endif
+  }
+};
Index: test/SemaCXX/warn-self-assign-field-builtin-warn-test.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-field-builtin-warn-test.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -wtest -verify %s
+
+struct C {
+  int a;
+
+  void f() {
+#ifndef SILENCE
+a = a; // expected-warning{{assigning field to itself}}
+#else
+// expected-no-diagnostics
+a = a;
+#endif
+  }
+};
Index: test/SemaCXX/warn-self-assign-builtin-warn-test.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-builtin-warn-test.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s

[PATCH] D45685: [Sema] Add -Wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 142627.
lebedev.ri added a comment.

- 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` ?


Repository:
  rC Clang

https://reviews.llvm.org/D45685

Files:
  docs/ReleaseNotes.rst
  docs/UsersManual.rst
  include/clang/AST/ASTDiagnostic.h
  include/clang/AST/CommentDiagnostic.h
  include/clang/Basic/Diagnostic.h
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/Driver/DriverDiagnostic.h
  include/clang/Frontend/FrontendDiagnostic.h
  include/clang/Lex/LexDiagnostic.h
  include/clang/Parse/ParseDiagnostic.h
  include/clang/Sema/SemaDiagnostic.h
  include/clang/Serialization/SerializationDiagnostic.h
  include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
  lib/Basic/DiagnosticIDs.cpp
  lib/Basic/Warnings.cpp
  lib/Sema/SemaExpr.cpp
  test/Sema/warn-self-assign-builtin-warn-tests.c
  test/SemaCXX/warn-self-assign-builtin-warn-tests.cpp
  test/SemaCXX/warn-self-assign-field-builtin-warn-tests.cpp
  test/SemaCXX/warn-self-assign-field-overloaded-warn-tests.cpp
  test/SemaCXX/warn-self-assign-overloaded-warn-tests.cpp
  tools/diagtool/DiagnosticNames.cpp
  utils/TableGen/ClangDiagnosticsEmitter.cpp

Index: utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -552,6 +552,11 @@
 else
   OS << ", false";
 
+if (R.getValueAsBit("HideInTests"))
+  OS << ", true";
+else
+  OS << ", false";
+
 // Category number.
 OS << ", " << CategoryIDs.getID(getDiagnosticCategory(, DGParentMap));
 OS << ")\n";
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -28,9 +28,9 @@
 // FIXME: Is it worth having two tables, especially when this one can get
 // out of sync easily?
 static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
-#define DIAG(ENUM,CLASS,DEFAULT_MAPPING,DESC,GROUP,   \
- SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY)\
-  { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
+#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR,  \
+ SHOWINSYSHEADER, HIDEINTESTS, CATEGORY)   \
+  {#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
 #include "clang/Basic/DiagnosticCommonKinds.inc"
 #include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
Index: test/SemaCXX/warn-self-assign-overloaded-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-overloaded-warn-tests.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-tests -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wtests -verify -DSILENCE %s
+
+struct S {};
+
+void f() {
+  S a;
+#ifndef SILENCE
+  a = a; // expected-warning{{explicitly assigning}}
+#else
+  // expected-no-diagnostics
+  a = a;
+#endif
+}
Index: test/SemaCXX/warn-self-assign-field-overloaded-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-field-overloaded-warn-tests.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wno-tests -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wtests -verify %s
+
+struct S {};
+
+struct C {
+  S a;
+
+  void f() {
+#ifndef SILENCE
+a = a; // expected-warning{{assigning field to itself}}
+#else
+// expected-no-diagnostics
+a = a;
+#endif
+  }
+};
Index: test/SemaCXX/warn-self-assign-field-builtin-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-field-builtin-warn-tests.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wno-tests -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wtests -verify %s
+
+struct C {
+  int a;
+
+  void f() {
+#ifndef SILENCE
+a = a; // expected-warning{{assigning field to itself}}
+#else
+// expected-no-diagnostics
+a = a;
+#endif
+  }
+};
Index: test/SemaCXX/warn-self-assign-builtin-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-builtin-warn-tests.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-tests -verify %s
+// RUN: 

[PATCH] D45685: [Sema] Add -Wtest global flag that silences -Wself-assign for overloaded operators.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: rjmccall, aaron.ballman, dblaikie, thakis, rsmith, 
chandlerc, brooksmoses.
Herald added a subscriber: klimek.

Credit for the idea goes to @rjmccall.
I'm not quite sure how to do it with `-wtest` (lowercase). so i just modelled 
it after `-Wsystem-headers`.

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

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


Repository:
  rC Clang

https://reviews.llvm.org/D45685

Files:
  docs/ReleaseNotes.rst
  docs/UsersManual.rst
  include/clang/AST/ASTDiagnostic.h
  include/clang/AST/CommentDiagnostic.h
  include/clang/Basic/Diagnostic.h
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/Driver/DriverDiagnostic.h
  include/clang/Frontend/FrontendDiagnostic.h
  include/clang/Lex/LexDiagnostic.h
  include/clang/Parse/ParseDiagnostic.h
  include/clang/Sema/SemaDiagnostic.h
  include/clang/Serialization/SerializationDiagnostic.h
  include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
  lib/Basic/DiagnosticIDs.cpp
  lib/Basic/Warnings.cpp
  lib/Sema/SemaExpr.cpp
  test/Sema/warn-self-assign-builtin-warn-tests.c
  test/SemaCXX/warn-self-assign-builtin-warn-tests.cpp
  test/SemaCXX/warn-self-assign-field-builtin-warn-tests.cpp
  test/SemaCXX/warn-self-assign-field-overloaded-warn-tests.cpp
  test/SemaCXX/warn-self-assign-overloaded-warn-tests.cpp
  tools/diagtool/DiagnosticNames.cpp
  utils/TableGen/ClangDiagnosticsEmitter.cpp

Index: utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -552,6 +552,11 @@
 else
   OS << ", false";
 
+if (R.getValueAsBit("HideInTests"))
+  OS << ", true";
+else
+  OS << ", false";
+
 // Category number.
 OS << ", " << CategoryIDs.getID(getDiagnosticCategory(, DGParentMap));
 OS << ")\n";
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -28,9 +28,9 @@
 // FIXME: Is it worth having two tables, especially when this one can get
 // out of sync easily?
 static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
-#define DIAG(ENUM,CLASS,DEFAULT_MAPPING,DESC,GROUP,   \
- SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY)\
-  { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
+#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR,  \
+ SHOWINSYSHEADER, HIDEINTESTS, CATEGORY)   \
+  {#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
 #include "clang/Basic/DiagnosticCommonKinds.inc"
 #include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
Index: test/SemaCXX/warn-self-assign-overloaded-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-overloaded-warn-tests.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wno-tests -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign -Wtests -verify -DSILENCE %s
+
+struct S {};
+
+void f() {
+  S a;
+#ifndef SILENCE
+  a = a; // expected-warning{{explicitly assigning}}
+#else
+  // expected-no-diagnostics
+  a = a;
+#endif
+}
Index: test/SemaCXX/warn-self-assign-field-overloaded-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-field-overloaded-warn-tests.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wno-tests -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wtests -verify %s
+
+struct S {};
+
+struct C {
+  S a;
+
+  void f() {
+#ifndef SILENCE
+a = a; // expected-warning{{assigning field to itself}}
+#else
+// expected-no-diagnostics
+a = a;
+#endif
+  }
+};
Index: test/SemaCXX/warn-self-assign-field-builtin-warn-tests.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-self-assign-field-builtin-warn-tests.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wno-tests -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -Wtests -verify %s
+
+struct C {
+  int a;
+
+  void f() {
+#ifndef SILENCE
+a = a; // expected-warning{{assigning field to itself}}
+#else
+// expected-no-diagnostics
+a = a;
+#endif
+  }
+};
Index: test/SemaCXX/warn-self-assign-builtin-warn-tests.cpp