Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

This revision was automatically updated to reflect the committed changes. Closed by commit rL267957: Avoid -Wshadow warnings about constructor parameters named after fields (authored by rnk). Changed prior to commit: http://reviews.llvm.org/D18271?vs=55359=55515#toc Repository: rL LLVM

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rsmith added a comment. LGTM http://reviews.llvm.org/D18271 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk updated this revision to Diff 55359. rnk added a comment. - address comments http://reviews.llvm.org/D18271 Files: include/clang/Basic/Diagnostic.h include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rsmith accepted this revision. This revision is now accepted and ready to land. Comment at: include/clang/Basic/DiagnosticGroups.td:341-342 @@ +340,4 @@ + +def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor">; +def ShadowFieldInConstructorModified :

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk updated this revision to Diff 55044. rnk added a comment. - Implement suggestions to avoid warning twice http://reviews.llvm.org/D18271 Files: include/clang/Basic/Diagnostic.h include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:6437-6445 @@ +6436,11 @@ + // Warn immediately if -Wshadow-field-in-constructor is set. + Diag(R.getNameLoc(), diag::warn_ctor_parm_shadows_field) + << D << FD << FD->getParent(); +

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk updated this revision to Diff 54206. rnk added a comment. - Address review comments - Add -Wshadow-all and -Wshadow-field-in-constructor, also address review comments - Warn twice under -Wshadow-all if a shadowing parameter is modified http://reviews.llvm.org/D18271 Files:

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk added inline comments. Comment at: lib/Sema/SemaDecl.cpp:6417-6425 @@ +6416,11 @@ +if (isa(NewDC) && isa(D)) { + if (Diags.isIgnored(diag::warn_ctor_parm_shadows_field, R.getNameLoc())) { +D = D->getCanonicalDecl(); +ShadowingDecls.insert({D, FD}); +

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:6417-6425 @@ +6416,11 @@ +if (isa(NewDC) && isa(D)) { + if (Diags.isIgnored(diag::warn_ctor_parm_shadows_field, R.getNameLoc())) { +D = D->getCanonicalDecl(); +ShadowingDecls.insert({D,

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk added a comment. ping http://reviews.llvm.org/D18271 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

On Thu, Apr 7, 2016 at 5:12 PM, David Blaikie wrote: > On Thu, Apr 7, 2016 at 5:05 PM, Reid Kleckner via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> rnk updated this revision to Diff 52982. >> rnk marked 3 inline comments as done. >> rnk added a comment. >> >> -

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

On Thu, Apr 7, 2016 at 5:12 PM, David Blaikie wrote: > On Thu, Apr 7, 2016 at 5:05 PM, Reid Kleckner via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> rnk updated this revision to Diff 52982. >> rnk marked 3 inline comments as done. >> rnk added a comment. >> >> -

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

On Thu, Apr 7, 2016 at 5:05 PM, Reid Kleckner via cfe-commits < cfe-commits@lists.llvm.org> wrote: > rnk updated this revision to Diff 52982. > rnk marked 3 inline comments as done. > rnk added a comment. > > - Add -Wshadow-all and -Wshadow-field-in-constructor, also address review > comments >

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk added inline comments. Comment at: lib/Sema/SemaDecl.cpp:6489-6490 @@ +6488,4 @@ + ShadowedDeclKind Kind = computeShadowedDeclKind(ShadowedDecl, OldDC); + Diag(Loc, diag::warn_modifying_shadowing_decl) << D << Kind << OldDC; + Diag(ShadowedDecl->getLocation(),

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk updated this revision to Diff 52983. rnk added a comment. - Add a test for -Wshadow-all http://reviews.llvm.org/D18271 Files: include/clang/Basic/Diagnostic.h include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk updated this revision to Diff 52982. rnk marked 3 inline comments as done. rnk added a comment. - Add -Wshadow-all and -Wshadow-field-in-constructor, also address review comments http://reviews.llvm.org/D18271 Files: include/clang/Basic/Diagnostic.h

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:1615 @@ -1614,2 +1614,3 @@ IdResolver.RemoveDecl(D); +ShadowingDecls.erase(D); } This'd be a good place to produce a -Wshadow-constructor or similar warning if D is still in the map.

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk added a comment. We had a conversation about this change around the office the other week, and people were concerned about false negatives like the trim_in_place one. Basically, I don't have time to discover all the ways you can modify your parameters: struct B { A a; B(A a) :

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

alexfh added a comment. Richard, is there anything else that blocks this patch? Comment at: lib/Sema/SemaDecl.cpp:6372 @@ +6371,3 @@ + if (isa(OldDC)) { +if (isa(ShadowedDecl)) + return SDK_Field; How about `return isa(ShadowedDecl) ? SDK_Field :

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk updated this revision to Diff 51697. rnk added a comment. - Address review comments, use a DenseMap instead of redoing lookup http://reviews.llvm.org/D18271 Files: include/clang/Basic/Diagnostic.h include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

alexfh added a comment. > Depending on how strictly you define "initialized" here, this is either > unhelpful or difficult to chpatterneck. s/chpatterneck/check/ http://reviews.llvm.org/D18271 ___ cfe-commits mailing list

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

alexfh added a comment. In http://reviews.llvm.org/D18271#382082, @rnk wrote: > In http://reviews.llvm.org/D18271#381769, @rsmith wrote: > > > I think a reasonable approach would be: "do not warn on shadowing if the > > idiom of naming a constructor parameter after a class member is used, and

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk added a comment. In http://reviews.llvm.org/D18271#381769, @rsmith wrote: > I think a reasonable approach would be: "do not warn on shadowing if the > idiom of naming a constructor parameter after a class member is used, and the > class member is initialized from that constructor

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

bcraig added a subscriber: mclow.lists. bcraig added a comment. > > Not dblaikie, but I've used the GCC version of -Wshadow on reasonably large > > code bases before. The ctor pattern that this is trying to squelch was > > certainly a significant portion of the warnings, particularly when

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rsmith added a comment. I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter, and all uses of the parameter in the constructor body

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

thakis added a comment. In http://reviews.llvm.org/D18271#381758, @bcraig wrote: > In http://reviews.llvm.org/D18271#381744, @thakis wrote: > > > FWIW we don't currently use this warning on Chromium because it's way to > > noisy. So something like this looks like a great change to me. > > > >

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

bcraig added a subscriber: bcraig. bcraig added a comment. In http://reviews.llvm.org/D18271#381744, @thakis wrote: > FWIW we don't currently use this warning on Chromium because it's way to > noisy. So something like this looks like a great change to me. > > dblaikie, are you aware of any

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

thakis added a subscriber: thakis. thakis added a comment. FWIW we don't currently use this warning on Chromium because it's way to noisy. So something like this looks like a great change to me. dblaikie, are you aware of any codebases that use this warning in its current form?

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

alexfh added a comment. In http://reviews.llvm.org/D18271#378196, @dblaikie wrote: > It's not just modifications of shadowed variables that are a problem - one of > the one's I'm concerned we should catch is: > > struct foo { > std::unique_ptr p; > foo(std::unique_ptr p) :

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

On Wed, Mar 23, 2016 at 11:03 AM, Alexander Kornienko via cfe-commits < cfe-commits@lists.llvm.org> wrote: > alexfh added a subscriber: alexfh. > alexfh added a comment. > > As a data point: I ran -Wshadow on our code base with a similar, but > simpler patch (http://reviews.llvm.org/D18395, just

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

alexfh added a comment. In http://reviews.llvm.org/D18271#381728, @alexfh wrote: > ... and then gradually adding back the cases that seem to be important to > flag ... Maybe as separate warnings. http://reviews.llvm.org/D18271 ___ cfe-commits

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

alexfh added a subscriber: alexfh. alexfh added a comment. As a data point: I ran -Wshadow on our code base with a similar, but simpler patch (http://reviews.llvm.org/D18395, just disables the warning on ctor parameters named after fields). It removes more than half of the hits (from ~100k

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk added a comment. I'm not sure your example is in scope for -Wshadow, though. Any function call that takes a non-const reference to the parameter could modify it. I guess I'm thinking something like: void trim_in_place(std::string ); struct A { std::string s; A(std::string s) :

[PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

rnk created this revision. rnk added reviewers: rsmith, rtrieu. rnk added a subscriber: cfe-commits. Emit a new warning if such parameters are modified in the body of the constructor. Fixes PR16088. http://reviews.llvm.org/D18271 Files: include/clang/Basic/DiagnosticSemaKinds.td

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

On Fri, Mar 18, 2016 at 11:08 AM, Reid Kleckner via cfe-commits < cfe-commits@lists.llvm.org> wrote: > rnk added a comment. > > I'm not sure your example is in scope for -Wshadow, though. Any function > call that takes a non-const reference to the parameter could modify it. Sure - my argument

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

dblaikie added a subscriber: dblaikie. dblaikie added a comment. It's not just modifications of shadowed variables that are a problem - one of the one's I'm concerned we should catch is: struct foo { std::unique_ptr p; foo(std::unique_ptr p) : p(std::move(p)) { f(*p); } };

Re: [PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

+Lang, because he was asking me recently about this improvement & thinking of chipping in On Fri, Mar 18, 2016 at 10:56 AM, David Blaikie via cfe-commits < cfe-commits@lists.llvm.org> wrote: > dblaikie added a subscriber: dblaikie. > dblaikie added a comment. > > It's not just modifications of