Avoid -Wshadow warnings about constructor parameters named after fields

[rnk]

Summary

Emit a new warning if such parameters are modified in the body of the
constructor.

Fixes PR16088.

[dblaikie]

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<int> p;
  foo(std::unique_ptr<int> p) : p(std::move(p)) {
    f(*p);
  }
};

[cfe-commits]

+Lang, because he was asking me recently about this improvement & thinking
of chipping in

• msg-16506-48.txt159 BDownload

[rnk]

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 &s);
struct A {
  std::string s;
  A(std::string s) : s(s) { trim_in_place(s); }
};

I think if we try to match that we'll have too many false positives.

I think your example would be better caught by something like -Wconsumed that looks for uses of objects that have been moved-from. That warning won't be confused by shadowing and will give a better diagnostic anyway.

[alexfh]

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 initially) and a random sample of ~100 removed removed hits didn't contain any issues that we'd like to still be warning about.

Ultimately, I think, this diagnostic should be made much less noisy to be actually useful. So maybe even starting with a removal of a larger subset of warnings (and then gradually adding back the cases that seem to be important to flag) would be better.

lib/Sema/SemaExpr.cpp

9887 ↗

(On Diff #51039)

Why this change?

[alexfh]

In D18271#381728, @alexfh wrote:

... and then gradually adding back the cases that seem to be important to flag ...

Maybe as separate warnings.

[alexfh]

In 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<int> p;
  foo(std::unique_ptr<int> p) : p(std::move(p)) {
    f(*p);
  }
};

This seems totally out of scope for -Wshadow, and seeing a -Wshadow warning on this wouldn't help (imagine a few tens of lines before the *p is used).

[thakis]

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?

[bcraig]

In 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 codebases that use this warning in its current form?

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 (old) boost headers got involved.

I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.

[thakis]

In D18271#381758, @bcraig wrote:

In 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 codebases that use this warning in its current form?

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 (old) boost headers got involved.

I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.

Did squelching this ctor pattern find any bugs? Do you know if boost would miss the warning firing in this case?

[rsmith]

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 would still be valid if it were declared const".

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).

(I agree that catching A(X x) : x(std::move(x)) { use(x); } is outside the scope of this change.)

lib/Sema/SemaDecl.cpp

6400 ↗

(On Diff #51039)

Instead of redoing class member lookup every time we check to see if a variable is modifiable, perhaps you could populate a set of shadowed-but-not-diagnosed VarDecl*s here? That way, you can also suppress the duplicate diagnostics if the variable is modified multiple times by removing it from the set.

lib/Sema/SemaExpr.cpp

9663 ↗

(On Diff #51039)

This query is actually quite expensive (more so than some or all of the other checks below).

[bcraig]

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 (old) boost headers got involved.

I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.

Did squelching this ctor pattern find any bugs? Do you know if boost would miss the warning firing in this case?

I don't recall fixing any bugs with the ctor pattern. I can only recall two different classes of things that I have fixed that were flagged with -Wshadow.

1. Long, horrible, deeply nested method, with a local that was trivially shadowing a different local in a larger scope.
2. Mutex lock guards and similar "sentry" objects. "std::unique_lock<std::mutex>(name_of_a_member);" The user wanted that to lock 'name_of_member' and release it at end of scope, but instead it declared a default constructed unique_lock called 'name_of_a_member' that shadows the member variable. I've had to fix this kind of bug a number of times, and it is my main justification for turning on -Wshadow in the first place. On that note, I'm really looking forward to the day I can say "auto guard = std::unique_lock(name_of_member);" to effectively get rid of this problem.

I doubt boost would miss the warning, and probably wouldn't even notice (barring overlapping list membership). If you're really wondering if boost would care / notice, @mclow.lists would have a better idea.

[rnk]

In 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 parameter, and all uses of the parameter in the constructor body would still be valid if it were declared const".

That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).

Would the reference binding check be somewhere in SemaInit.cpp? I don't see an obvious spot to do this check at first glance.

(I agree that catching A(X x) : x(std::move(x)) { use(x); } is outside the scope of this change.)

Wouldn't the proposed check for non-const references find this example, though?

lib/Sema/SemaDecl.cpp

6400 ↗

(On Diff #51039)

That sounds like a good idea. I could even leave it empty if we're not doing -Wshadow.

lib/Sema/SemaExpr.cpp

9663 ↗

(On Diff #51039)

This check has to be faster than name lookup, right? Maybe do it before that?

[alexfh]

In D18271#382082, @rnk wrote:

In 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 parameter,

Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck. Consider following examples:

class C1 {
  T field;
public:
  void set_field(T f) {
    field = f;
    // do something else...
  }
  C1(T field) { set_field(field); }
};

class C2 {
  T1 f1;
  T2 f2;
public:
  C2(T1 f1, T2 f2) {
    // Some form of initialization of `this->field` from `field` that is not just a copy construction/copy/move,
    // e.g. a method call or an assignment of a single member:
    this->f1.a = f1.a;
    this->f2.CopyFrom(f2);
  }
};

I've seen these patterns being used in the real code, and they don't seem to be dangerous or otherwise worth being flagged by this diagnostic.

and all uses of the parameter in the constructor body would still be valid if it were declared const".

That might also be overly restrictive. Consider a variant of set_field above that takes a T& (which is weird, but it doesn't seem like -Wshadow is the right warning in this case).

That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).

See above. There are many cases, when at least the latter pattern is used in a reasonable way.

[alexfh]

Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck.

s/chpatterneck/check/

[rnk]

• Address review comments, use a DenseMap instead of redoing lookup

[alexfh]

Richard, is there anything else that blocks this patch?

lib/Sema/SemaDecl.cpp

6372 ↗

(On Diff #51697)

How about return isa<FieldDecl>(ShadowedDecl) ? SDK_Field : SDK_StaticMember;?

6376 ↗

(On Diff #51697)

No else after return, please.

[rnk]

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) : a(a) { // modifies parameter, not member
    a.a = 42;
    modify_in_place(&a);
    a.setMember(3);
  }
};

These kinds of bugs are uncommon, but the existing -Wshadow protects users against this today. I think it'll be OK if we give these users an extra -Wshadow-constructor preserves the more strict behavior.

[rsmith]

lib/Sema/SemaDecl.cpp

1615 ↗

(On Diff #51697)

This'd be a good place to produce a -Wshadow-constructor or similar warning if D is still in the map.

6483 ↗

(On Diff #51697)

Maybe map to the canonical decl before using the Decl* as a key? (This doesn't matter for now, but we'll likely want to extend this to other kinds of shadowing in the future.)

6489–6490 ↗

(On Diff #51697)

Could be useful to also note the location of the shadowing decl.

[rnk]

• Add -Wshadow-all and -Wshadow-field-in-constructor, also address review comments

[rnk]

• Add a test for -Wshadow-all

lib/Sema/SemaDecl.cpp

6489–6490 ↗

(On Diff #51697)

Sure, done. I reused and renamed a note from uninit variable diagnostics.

[rnk]

ping

[rsmith]

lib/Sema/SemaDecl.cpp

6417–6425 ↗

(On Diff #52983)

I would prefer for us to produce the -Wshadow "modifying shadowed" warning where it applies, even when -Wshadow-all is enabled. The usual model is to act as if -W* flags are just a filter over the diagnostic output, and this diverges from that.

Is the goal to ensure that -Wshadow-field-in-constructor diagnoses all constructor parameter / field shadowing, even if -Wshadow is not enabled?

[rnk]

lib/Sema/SemaDecl.cpp

6417–6425 ↗

(On Diff #52983)

Well, if we do that we'll get two warnings about the shadowed field: one on the site of the parameter declaration and another on the modification. That felt a bit redundant.

I can remove the if/else and we'll get the behavior you're asking for. Most users will not use -Wshadow-all, and even if they do they'll only see the double diagnostic in very rare cases.

[rnk]

• 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

[rsmith]

lib/Sema/SemaDecl.cpp

6437–6445 ↗

(On Diff #54206)

Here's what I was thinking:

• The new warning goes in its own group, say -Wshadow-field-in-constructor-modified
• The new warning group is part of both -Wshadow and -Wshadow-field-in-constructor
• We produce the -Wshadow-field-in-constructor warning from ActOnPopScope whenever we remove something from the map.

This means that our behavior is as if -W flags filter our warning output, and we only get one warning for each instance of shadowing, and -Wshadow-field-in-constructor can be used to control all constructor-parameter-shadows-field warnings (and even obscure cases like -Wshadow -Wno-shadow-field-in-constructor do what they look like they should do).

[rnk]

• Implement suggestions to avoid warning twice

[rsmith]

include/clang/Basic/DiagnosticGroups.td

341–342 ↗

(On Diff #55044)

Should ShadowFieldInConstructorModified be a subgroup of ShadowFieldInConstructor? I'd expect -Wshadow-field-in-constructor to enable both.

lib/Sema/SemaDecl.cpp

6444 ↗

(On Diff #55044)

existance -> existence

[rnk]

• address comments

[rsmith]

LGTM

cfe/trunk/include/clang/Basic/Diagnostic.h

DB.AddTaggedVal(reinterpret_cast<intptr_t>(II),
                   DiagnosticsEngine::ak_identifierinfo);
   return DB;
 }
 
 // Adds a DeclContext to the diagnostic. The enable_if template magic is here
 // so that we only match those arguments that are (statically) DeclContexts;
 // other arguments that derive from DeclContext (e.g., RecordDecls) will not
 // match.
-template<typename T>
-inline
-typename std::enable_if<std::is_same<T, DeclContext>::value,
-                        const DiagnosticBuilder &>::type
+template <typename T>
+inline typename std::enable_if<
+    std::is_same<typename std::remove_const<T>::type, DeclContext>::value,
 operator<<(const DiagnosticBuilder &DB, T *DC) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(DC),
                   DiagnosticsEngine::ak_declcontext);
   return DB;
 }
 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            SourceRange R) {

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def BindToTemporaryCopy : DiagGroup<"bind-to-temporary-copy",
                                     [CXX98CompatBindToTemporaryCopy]>;
 def SelfAssignmentField : DiagGroup<"self-assign-field">;
 def SelfAssignment : DiagGroup<"self-assign", [SelfAssignmentField]>;
 def SelfMove : DiagGroup<"self-move">;
 def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">;
 def Sentinel : DiagGroup<"sentinel">;
 def MissingMethodReturnType : DiagGroup<"missing-method-return-type">;
-def Shadow : DiagGroup<"shadow">;
+
+def ShadowFieldInConstructorModified : DiagGroup<"shadow-field-in-constructor-modified">;
+def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor",
+                                         [ShadowFieldInConstructorModified]>;
+
+// -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the
+// shadowing that we think is unsafe.
+def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified]>;
+def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor]>;
+
 def Shorten64To32 : DiagGroup<"shorten-64-to-32">;
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
 def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 def warn_decl_shadow :
   Warning<"declaration shadows a %select{"
           "local variable|"
           "variable in %2|"
           "static data member of %2|"
           "field of %2}1">,
   InGroup<Shadow>, DefaultIgnore;
+def warn_ctor_parm_shadows_field:
+  Warning<"constructor parameter %0 shadows the field %1 of %2">,
+  InGroup<ShadowFieldInConstructor>, DefaultIgnore;
+def warn_modifying_shadowing_decl :
+  Warning<"modifying constructor parameter %0 that shadows a "
+          "field of %1">,
+  InGroup<ShadowFieldInConstructorModified>, DefaultIgnore;
+
 
 // C++ using declarations
 def err_using_requires_qualname : Error<
   "using declaration requires a qualified name">;
 def err_using_typename_non_type : Error<
   "'typename' keyword used on a non-type">;
 def err_using_dependent_value_is_type : Error<
   "dependent using declaration resolved to type without 'typename'">;
def warn_sometimes_uninit_var : Warning<
   "switch %3 is taken|"
   "its declaration is reached|"
   "%3 is called}2">,
   InGroup<UninitializedSometimes>, DefaultIgnore;
 def warn_maybe_uninit_var : Warning<
   "variable %0 may be uninitialized when "
   "%select{used here|captured by block}1">,
   InGroup<UninitializedMaybe>, DefaultIgnore;
-def note_uninit_var_def : Note<"variable %0 is declared here">;
+def note_var_declared_here : Note<"variable %0 is declared here">;
 def note_uninit_var_use : Note<
   "%select{uninitialized use occurs|variable is captured by block}0 here">;
 def warn_uninit_byref_blockvar_captured_by_block : Warning<
   "block pointer variable %0 is uninitialized when captured by block">,
   InGroup<Uninitialized>, DefaultIgnore;
 def note_block_var_fixit_add_initialization : Note<
   "did you mean to use __block %0?">;
 def note_in_omitted_aggregate_initializer : Note<

cfe/trunk/include/clang/Sema/Sema.h

diagnoseIgnoredQualifiers(unsigned DiagID, unsigned Quals,
                             SourceLocation VolatileQualLoc = SourceLocation(),
                             SourceLocation RestrictQualLoc = SourceLocation(),
                             SourceLocation AtomicQualLoc = SourceLocation());
 
   static bool adjustContextForLocalExternDecl(DeclContext *&DC);
   void DiagnoseFunctionSpecifiers(const DeclSpec &DS);
   void CheckShadow(Scope *S, VarDecl *D, const LookupResult& R);
   void CheckShadow(Scope *S, VarDecl *D);
+
+  /// Warn if 'E', which is an expression that is about to be modified, refers
+  /// to a shadowing declaration.
+  void CheckShadowingDeclModification(Expr *E, SourceLocation Loc);
+
+private:
+  /// Map of current shadowing declarations to shadowed declarations. Warn if
+  /// it looks like the user is trying to modify the shadowing declaration.
+  llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;
+
+public:
   void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange);
   void handleTagNumbering(const TagDecl *Tag, Scope *TagScope);
   void setTagNameForLinkagePurposes(TagDecl *TagFromDeclSpec,
                                     TypedefNameDecl *NewTD);
   void CheckTypedefForVariablyModifiedType(Scope *S, TypedefNameDecl *D);
   NamedDecl* ActOnTypedefDeclarator(Scope* S, Declarator& D, DeclContext* DC,
                                     TypeSourceInfo *TInfo,
                                     LookupResult &Previous);

cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Use.getUser())) {
     else
       DiagUninitUse(S, VD, Use, true);
   }
 
   // Report where the variable was declared when the use wasn't within
   // the initializer of that declaration & we didn't already suggest
   // an initialization fixit.
   if (!SuggestInitializationFixit(S, VD))
-    S.Diag(VD->getLocStart(), diag::note_uninit_var_def)
+    S.Diag(VD->getLocStart(), diag::note_var_declared_here)
       << VD->getDeclName();
 
   return true;
 }
 
 namespace {
   class FallthroughMapper : public RecursiveASTVisitor<FallthroughMapper> {
   public:

cfe/trunk/lib/Sema/SemaDecl.cpp

if (!S->hasUnrecoverableErrorOccurred()) {
       DiagnoseUnusedDecl(D);
       if (const auto *RD = dyn_cast<RecordDecl>(D))
         DiagnoseUnusedNestedTypedefs(RD);
     }
     
     // If this was a forward reference to a label, verify it was defined.
     if (LabelDecl *LD = dyn_cast<LabelDecl>(D))
       CheckPoppedLabel(LD, *this);
-    
-    // Remove this name from our lexical scope.
+    // Remove this name from our lexical scope, and warn on it if we haven't
+    // already.
     IdResolver.RemoveDecl(D);
+    auto ShadowI = ShadowingDecls.find(D);
+    if (ShadowI != ShadowingDecls.end()) {
+      if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second)) {
+        Diag(D->getLocation(), diag::warn_ctor_parm_shadows_field)
+            << D << FD << FD->getParent();
+        Diag(FD->getLocation(), diag::note_previous_declaration);
+      }
+      ShadowingDecls.erase(ShadowI);
+    }
   }
 }
 
 /// \brief Look for an Objective-C class in the translation unit.
 ///
 /// \param Id The name of the Objective-C class we're looking for. If
 /// typo-correction fixes this name, the Id will be updated
 /// to the fixed name.
if (NewVD->isInvalidDecl())
       NewTemplate->setInvalidDecl();
     ActOnDocumentableDecl(NewTemplate);
     return NewTemplate;
   }
 
   return NewVD;
 }
 
+/// Enum describing the %select options in diag::warn_decl_shadow.
+enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field };
+
+/// Determine what kind of declaration we're shadowing.
+static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl,
+                                                const DeclContext *OldDC) {
+  if (isa<RecordDecl>(OldDC))
+    return isa<FieldDecl>(ShadowedDecl) ? SDK_Field : SDK_StaticMember;
+  return OldDC->isFileContext() ? SDK_Global : SDK_Local;
+}
+
 /// \brief Diagnose variable or built-in function shadowing.  Implements
 /// -Wshadow.
 ///
 /// This method is called whenever a VarDecl is added to a "useful"
 /// scope.
 ///
 /// \param S the scope in which the shadowing name is being declared
 /// \param R the lookup of the name
void Sema::CheckShadow(Scope *S, VarDecl *D, const LookupResult& R) {
   // Only diagnose if we're shadowing an unambiguous field or variable.
   if (R.getResultKind() != LookupResult::Found)
     return;
 
   NamedDecl* ShadowedDecl = R.getFoundDecl();
   if (!isa<VarDecl>(ShadowedDecl) && !isa<FieldDecl>(ShadowedDecl))
     return;
 
+  if (FieldDecl *FD = dyn_cast<FieldDecl>(ShadowedDecl)) {
     // Fields are not shadowed by variables in C++ static methods.
-  if (isa<FieldDecl>(ShadowedDecl))
     if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewDC))
       if (MD->isStatic())
         return;
 
+    // Fields shadowed by constructor parameters are a special case. Usually
+    // the constructor initializes the field with the parameter.
+    if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D)) {
+      // Remember that this was shadowed so we can either warn about its
+      // modification or its existence depending on warning settings.
+      D = D->getCanonicalDecl();
+      ShadowingDecls.insert({D, FD});
+      return;
+    }
+  }
+
   if (VarDecl *shadowedVar = dyn_cast<VarDecl>(ShadowedDecl))
     if (shadowedVar->isExternC()) {
       // For shadowing external vars, make sure that we point to the global
       // declaration, not a locally scoped extern declaration.
       for (auto I : shadowedVar->redecls())
         if (I->isFileVarDecl()) {
           ShadowedDecl = I;
           break;
if (NewDC && NewDC->isRecord()) {
     // TODO: should we warn about static data members shadowing
     // static data members from base classes?
     
     // TODO: don't diagnose for inaccessible shadowed members.
     // This is hard to do perfectly because we might friend the
     // shadowing context, but that's just a false negative.
   }
 
-  // Determine what kind of declaration we're shadowing.
-
-  // The order must be consistent with the %select in the warning message.
-  enum ShadowedDeclKind { Local, Global, StaticMember, Field };
-  ShadowedDeclKind Kind;
-  if (isa<RecordDecl>(OldDC)) {
-    if (isa<FieldDecl>(ShadowedDecl))
-      Kind = Field;
-    else
-      Kind = StaticMember;
-  } else if (OldDC->isFileContext()) {
-    Kind = Global;
-  } else {
-    Kind = Local;
-  }
 
   DeclarationName Name = R.getLookupName();
 
   // Emit warning and note.
   if (getSourceManager().isInSystemMacro(R.getNameLoc()))
     return;
+  ShadowedDeclKind Kind = computeShadowedDeclKind(ShadowedDecl, OldDC);
   Diag(R.getNameLoc(), diag::warn_decl_shadow) << Name << Kind << OldDC;
   Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
 }
 
 /// \brief Check -Wshadow without the advantage of a previous lookup.
 void Sema::CheckShadow(Scope *S, VarDecl *D) {
   if (Diags.isIgnored(diag::warn_decl_shadow, D->getLocation()))
     return;
 
   LookupResult R(*this, D->getDeclName(), D->getLocation(),
                  Sema::LookupOrdinaryName, Sema::ForRedeclaration);
   LookupName(R, S);
   CheckShadow(S, D, R);
 }
 
+/// Check if 'E', which is an expression that is about to be modified, refers
+/// to a constructor parameter that shadows a field.
+void Sema::CheckShadowingDeclModification(Expr *E, SourceLocation Loc) {
+  // Quickly ignore expressions that can't be shadowing ctor parameters.
+  if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
+    return;
+  E = E->IgnoreParenImpCasts();
+  auto *DRE = dyn_cast<DeclRefExpr>(E);
+  if (!DRE)
+    return;
+  const NamedDecl *D = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
+  auto I = ShadowingDecls.find(D);
+  if (I == ShadowingDecls.end())
+    return;
+  const NamedDecl *ShadowedDecl = I->second;
+  const DeclContext *OldDC = ShadowedDecl->getDeclContext();
+  Diag(Loc, diag::warn_modifying_shadowing_decl) << D << OldDC;
+  Diag(D->getLocation(), diag::note_var_declared_here) << D;
+  Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
+
+  // Avoid issuing multiple warnings about the same decl.
+  ShadowingDecls.erase(I);
+}
+
 /// Check for conflict between this global or extern "C" declaration and
 /// previous global or extern "C" declarations. This is only used in C++.
 template<typename T>
 static bool checkGlobalOrExternCConflict(
     Sema &S, const T *ND, bool IsGlobal, LookupResult &Previous) {
   assert(S.getLangOpts().CPlusPlus && "only C++ has extern \"C\"");
   NamedDecl *Prev = S.findLocallyScopedExternCDecl(ND->getDeclName());
 

cfe/trunk/lib/Sema/SemaExpr.cpp

static void DiagnoseConstAssignment(Sema &S, const Expr *E,
   // Can't determine a more specific message, so display the generic error.
   S.Diag(Loc, diag::err_typecheck_assign_const) << ExprRange << ConstUnknown;
 }
 
 /// CheckForModifiableLvalue - Verify that E is a modifiable lvalue.  If not,
 /// emit an error and return true.  If so, return false.
 static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
   assert(!E->hasPlaceholderType(BuiltinType::PseudoObject));
+
+  S.CheckShadowingDeclModification(E, Loc);
+
   SourceLocation OrigLoc = Loc;
   Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context,
                                                               &Loc);
   if (IsLV == Expr::MLV_ClassTemporary && IsReadonlyMessage(E, S))
     IsLV = Expr::MLV_InvalidMessageExpression;
   if (IsLV == Expr::MLV_Valid)
     return false;
 

cfe/trunk/test/SemaCXX/warn-shadow.cpp

-// RUN: %clang_cc1 -verify -fsyntax-only -Wshadow %s
+// RUN: %clang_cc1 -verify -fsyntax-only -Wshadow-all %s
 
 namespace {
   int i; // expected-note {{previous declaration is here}}
 }
 
 namespace one {
 namespace two {
   int j; // expected-note {{previous declaration is here}}

 void foo() {
   int i; // expected-warning {{declaration shadows a variable in namespace '(anonymous)'}}
   int j; // expected-warning {{declaration shadows a variable in namespace 'one::two'}}
   int m;
 }
 
 class A {
   static int data; // expected-note {{previous declaration}}
-  int field; // expected-note {{previous declaration}}
+  // expected-note@+1 {{previous declaration}}
+  int field;
+  int f1, f2, f3, f4; // expected-note 8 {{previous declaration is here}}
+
+  // The initialization is safe, but the modifications are not.
+  A(int f1, int f2, int f3, int f4) // expected-note-re 4 {{variable 'f{{[0-4]}}' is declared here}}
+    : f1(f1) {
+    f1 = 3; // expected-warning {{modifying constructor parameter 'f1' that shadows a field of 'A'}}
+    f1 = 4; // one warning per shadow
+    f2++; // expected-warning {{modifying constructor parameter 'f2' that shadows a field of 'A'}}
+    --f3; // expected-warning {{modifying constructor parameter 'f3' that shadows a field of 'A'}}
+    f4 += 2; // expected-warning {{modifying constructor parameter 'f4' that shadows a field of 'A'}}
+  }
+
+  // The initialization is safe, but the modifications are not.
+  // expected-warning-re@+1 4 {{constructor parameter 'f{{[0-4]}}' shadows the field 'f{{[0-9]}}' of 'A'}}
+  A(int f1, int f2, int f3, int f4, double overload_dummy) {}
 
   void test() {
     char *field; // expected-warning {{declaration shadows a field of 'A'}}
     char *data; // expected-warning {{declaration shadows a static data member of 'A'}}
   }
 };
 
 // TODO: this should warn, <rdar://problem/5018057>