aaron.ballman added a comment. In D129202#3634646 <https://reviews.llvm.org/D129202#3634646>, @njames93 wrote:
> In D129202#3633618 <https://reviews.llvm.org/D129202#3633618>, @aaron.ballman > wrote: > >> Thank you for working on this, I think it's a nice new diagnostic. You >> should add a release note to describe it. One question I have is about how >> chatty this is over real world code, especially older code bases. There used >> to be two prevailing ways to silence the "unused variable" warnings you >> would get when a variable is used in an `assert`: `(void)whatever;` and >> `whatever = whatever;`; I'm worried the latter case is still prevalent >> enough that having this warning on by default would be a problem, but I'm >> also optimistic my worry is unfounded. > > It's not adding a new warning, so chattiness of this patch shouldn't really > be much of an issue. It's not triggering a diagnostic in a situation we didn't used to diagnose, but it is doubling the number of diagnostics emitted in some circumstances. So it's not a new warning, but chattiness is still a valid concern. We don't want to swamp the user with diagnostic messages. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:14656 + + // Only check the fields declared in Parent, without traversing base classes + auto Field = llvm::find_if( ---------------- njames93 wrote: > aaron.ballman wrote: > > Why wouldn't we want to look through base classes for a member there? > > Considering a case like: > > ``` > > struct Base { > > int X; > > }; > > > > struct Derived : Base { > > Derived(int X) { X = X; } // Oooooops > > }; > > ``` > > it seems like we would want to use a real lookup operation instead of the > > fields of the class. Or did you try that and found it was a performance > > concern? > Once you start getting into deep class hierarchies we start to lose > confidence that the fix is what the user intended. Then there is also the > issue of access where the field may not even be accessible in Derived and it > just adds complexity that I'm not sure is really warranted. > Once you start getting into deep class hierarchies we start to lose > confidence that the fix is what the user intended. Why? If what we find is a variable (or data member), > Then there is also the issue of access where the field may not even be > accessible in Derived and it just adds complexity that I'm not sure is really > warranted. The complexity question is certainly something we should keep in mind, but I'm not really sure that using a `Lookup` method on `Sema` adds a significant amount of complexity, either. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:14661-14664 + if (Field != Parent->fields().end()) + S.Diag(LHSDeclRef->getBeginLoc(), diag::note_self_assign_insert_this) + << *Field + << FixItHint::CreateInsertion(LHSDeclRef->getBeginLoc(), "this->"); ---------------- njames93 wrote: > aaron.ballman wrote: > > Use of a note like this isn't awful, but I'm not super happy that the note > > always shows up on the same line as the warning. It seems like it would be > > cleaner (in terms of the user experience) to use a `%select` on the warning > > diagnostic to decide whether to add the "did you mean" information, and > > associate the fix directly with the warning. However, there's a slight > > functional difference with that approach which may matter to you: fixits > > attached to a note are not automatically applied when in fixit mode, but > > they are when attached to a warning or error. I don't know a reason why > > applying the fix-it automatically would be a bad thing in this case, so use > > of a note loses a bit of useful functionality. > That is exactly the reason for the note. Because we aren't fixing invalid > code(self assignment is suspicious but not an error) and the fix has a > noticeable change of behaviour, it doesn't seem wise to automatically apply > the fix-it. > I don't have a plan down the line to implement this, but what if the intent > of the programmer was actually a near miss, like in the, heavily contrived, > example below. > ```lang=c++ > struct Foo { > int Bar; > void Stuff(int Bar) { > int bar; > // ... > Bar = Bar; // Did they intend bar = Bar or this->Bar = Bar? > } > }; > ``` > > That is exactly the reason for the note. Because we aren't fixing invalid > code(self assignment is suspicious but not an error) and the fix has a > noticeable change of behaviour, it doesn't seem wise to automatically apply > the fix-it. Hmmm, I agree with you that self assignment is suspicious but not an error. However, using a K&R C function declaration is also suspicious but not an error; we still attach a fix-it to the warning for it: https://godbolt.org/z/afPr45Pd7. Self-assignment warnings are off-by-default (same as strict prototype warnings), so it seems reasonable to me that if a user opts into the diagnostic, they get the fixes on the warnings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129202/new/ https://reviews.llvm.org/D129202 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
