Ignore shadowing for declarations coming from within macros.

[djasper]

Summary

The case, I am particularly interested in is:

#define A(x)              \
  ...                     \
  if (...) {              \
    int SomeVariable = 1; \
    ...;                  \
  }

Here, SomeVariable never leaves the scope of the macro and at the same
time, it is very unlikely, that the macro code itself intended to use some
other declaration that is then shadowed by SomeVariable.

The patch currently disables -Wshadow for all declarations inside a
macro, but I am happy to make it stricter.

[kimgr]

I've seen this in the wild:

#define LOG(m) \
    {  \
      std::ostringstream os;  \
      os << m << "\n";  \
      LogWrite(os.str());  \
    }

auto os = GetOSName();
LOG("The OS is " << os);

It looks like your patch would miss this case. Not sure if current Clang catches it either, though, I don't remember the exact symptoms when we found this.

[djasper]

I'd be ok, with missing that case, but I am happy to hear more opinions.

[klimek]

Your patch description sounds like you're only switching the warning off when the scope ends in a macro, but the patch looks different. For example:

#define M int x

int x;
void f() {
  M;
  x = 42; // the user probably meant ::x here
}

would also be suppressed now, right? It seems like we already have the DeclContext (which is the scope afaik), so NewDC->getEndLoc().isMacroID() might solve that?

[djasper]

I don't understand how my patch description says that. I am trying to be very explicit about that fact that it doesn't, see last paragraph.

I have thought about what you are suggesting, but

• A DeclContext doesn't have getEndLoc().
• The DeclContext NewDC here is usually the FunctionDecl, which doesn't help. I think you are thinking about the parent CompoundStmt in terms of AST matching. Not sure there is something like that in the AST itself.

[klimek]

Could we perhaps only suppress the warning when we shadow a variable within the same declcontext? Generally, with macros, shadowing local variables is more surprising.

[djasper]

Updated to be a bit more strict (warn if declarations from the same context get
shadowed).

[djasper]

Like this?

[kimgr]

Any chance you could try my example too? It seems like this approach might catch it.

But does the new approach really do anything different for your original example?

[djasper]

Yes, your example would work, too. The specific use case I have is where we are shadowing global variables.

[lebedev.ri]

How does this relate to the gcc behavior?
I suspect not everyone would want to have this relaxed -Wshadow mode.
Perhaps it could be hidden under some new flag, which is not going to be automatically enabled by -Weverything.
Like, -Wshadow-in-macros which does nothing compared to -Wshadow, and then -Wno-shadow-in-macros which enables this relaxed mode?

[rsmith]

I think the ideal solution would be something like:

If a variable produced by a macro expansion shadows a variable not produced by a macro expansion, do not warn immediately. Instead, warn if we see a use of the inner variable in a context that is not produced by the same macro expansion (more broadly: for each point of use, if there is no macro expansion that contains both the point of declaration of the inner variable and all points of use, then warn). As an approximation to this, we could suppress the warning only if the (start of the) scope containing the inner variable and the inner variable itself were produced by the same macro expansion.

lib/Sema/SemaDecl.cpp

 /// scope.
 ///
 /// \param ShadowedDecl the declaration that is shadowed by the given variable
 /// \param R the lookup of the name
 ///
 void Sema::CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
                        const LookupResult &R) {
   DeclContext *NewDC = D->getDeclContext();
+  DeclContext *OldDC = ShadowedDecl->getDeclContext()->getRedeclContext();
+
+  if (D->getLocation().isMacroID() && NewDC != OldDC)
+    return;
 
   if (FieldDecl *FD = dyn_cast<FieldDecl>(ShadowedDecl)) {
     // Fields are not shadowed by variables in C++ static methods.
     if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewDC))
       if (MD->isStatic())
         return;
 
     // Fields shadowed by constructor parameters are a special case. Usually
if (shadowedVar->isExternC()) {
       // declaration, not a locally scoped extern declaration.
       for (auto I : shadowedVar->redecls())
         if (I->isFileVarDecl()) {
           ShadowedDecl = I;
           break;
         }
     }
 
-  DeclContext *OldDC = ShadowedDecl->getDeclContext()->getRedeclContext();
-
   unsigned WarningDiag = diag::warn_decl_shadow;
   SourceLocation CaptureLoc;
   if (isa<VarDecl>(D) && isa<VarDecl>(ShadowedDecl) && NewDC &&
       isa<CXXMethodDecl>(NewDC)) {
     if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
       if (RD->isLambda() && OldDC->Encloses(NewDC->getLexicalParent())) {
         if (RD->getLambdaCaptureDefault() == LCD_None) {
           // Try to avoid warnings for lambdas with an explicit capture list.

test/SemaCXX/warn-shadow.cpp

 }
 
 void test10() {
   using bob=int; // no warning
   using bob1=int; // expected-warning {{declaration shadows a typedef in the global namespace}}
   using bob2=int; // expected-warning {{declaration shadows a type alias in the global namespace}}
 }
 
+void macro() {
+  int x; // expected-note 1 {{previous declaration is here}}
+#define A(a) { int a = 0; }
+  A(x); // expected-warning {{declaration shadows a local variable}}
+  A(i);
+}
+
 namespace rdar29067894 {
 
 void avoidWarningWhenRedefining(int b) { // expected-note {{previous definition is here}}
   int a = 0; // expected-note {{previous definition is here}}
   int a = 1; // expected-error {{redefinition of 'a'}}
   int b = 2; // expected-error {{redefinition of 'b'}}
 
   using c=char; // expected-note {{previous definition is here}}
void avoidWarningWhenRedefining(int b) { // expected-note {{previous definition is here}}
   int h; // expected-error {{redefinition of 'h'}}
 
   int k; // expected-note {{previous definition is here}}
   typedef int k; // expected-error {{redefinition of 'k'}}
 
   using l=char; // no warning or error.
   using l=char; // no warning or error.
   typedef char l; // no warning or error.
- 
-  typedef char n; // no warning or error. 
+  typedef char n; // no warning or error.
   typedef char n; // no warning or error.
   using n=char; // no warning or error.
 }
 
-}
+} // namespace rdar29067894
 
 extern "C" {
 typedef int externC; // expected-note {{previous declaration is here}}
 }
 void handleLinkageSpec() {
   typedef void externC; // expected-warning {{declaration shadows a typedef in the global namespace}}
 }