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 her
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?
Reposit
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
_
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 pro
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 tha
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/
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
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://li
+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
> inl
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 p
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.
>
>
>
>
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 sor
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 li
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 head
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 t
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 oth
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
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
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 afte
19 matches
Mail list logo