This is an archive of the discontinued LLVM Phabricator instance.

Ignore shadowing for declarations coming from within macros.
Needs ReviewPublic

Authored by djasper on Jul 23 2017, 11:14 PM.

Details

Reviewers
bkramer
klimek
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.

Event Timeline

djasper created this revision.Jul 23 2017, 11:14 PM
kimgr added a subscriber: kimgr.Jul 23 2017, 11:29 PM

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.

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

klimek edited edge metadata.Jul 24 2017, 12:40 AM

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?

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.

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 this revision to Diff 107885.Jul 24 2017, 4:54 AM

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

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?

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

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 added a subscriber: rsmith.Dec 17 2018, 11:40 AM

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.