This is an archive of the discontinued LLVM Phabricator instance.

Enhance -Wshadow to warn when shadowing typedefs or type aliases
ClosedPublic

Authored by ahmedasadi on Mar 22 2017, 12:55 AM.

Diff Detail

Event Timeline

ahmedasadi created this revision.Mar 22 2017, 12:55 AM

Please update the patch to include the full context (git diff -U999999).

Updated diff to include context.

arphaman added inline comments.Mar 22 2017, 11:20 AM
lib/Sema/SemaDecl.cpp
6777–6783

Why is the change to this if necessary? It doesn't seem that related to the main change.

ahmedasadi added inline comments.Mar 22 2017, 2:22 PM
lib/Sema/SemaDecl.cpp
6777–6783

VarDecl overrides getCanonicalDecl() to return a VarDecl*. As the type of D was changed from VarDecl* to NamedDecl*, getCanonicalDecl() now returns a NamedDecl*.

I created a new ParmVarDecl variable so getCanonicalDecl() returns a VarDecl* which can be inserted into ShadowingDecls.

Perhaps it might be better to just cast D->getCanonicalDecl() to a VarDecl when inserting it into ShadowingDecls?

arphaman added inline comments.Mar 23 2017, 4:23 AM
lib/Sema/SemaDecl.cpp
5547

You don't need to use {} braces here.

6777–6783

I see, thanks for the explanation. The change is fine then.

6778

You can use const auto here.

test/SemaCXX/warn-shadow.cpp
65

It looks like previously this wouldn't have been a warning. Should we really warn about local variables that shadow typedef names?

ahmedasadi marked 3 inline comments as done.Mar 28 2017, 12:56 PM
ahmedasadi added inline comments.
test/SemaCXX/warn-shadow.cpp
65

GCC does, though I agree the warning isn't that helpful as a variable typically can't be used in place of a type (only exception I can think of is sizeof).

I'll modify the patch to only warn when typedefs / type aliases shadow each other.

ahmedasadi marked 4 inline comments as done.
rnk accepted this revision.Mar 30 2017, 5:46 PM

Looks good to me

lib/Sema/SemaDecl.cpp
6717–6718

I would recommend reordering these checks. Examining our name lookup result is going to be way faster than asking the diagnostic machinery if a warning is enabled. That code is unfortunately quite slow.

This revision is now accepted and ready to land.Mar 30 2017, 5:46 PM
ahmedasadi updated this revision to Diff 93583.Mar 30 2017, 9:13 PM
ahmedasadi marked an inline comment as done.

Re-ordered the checks in shouldWarnIfShadowedDecl as suggested by rnk.

Thanks for reviewing. Would you be able to commit this patch for me, as I do not have commit access?

Thanks!

I'll commit it for you right now.

This revision was automatically updated to reflect the committed changes.
EricWF added a subscriber: EricWF.Apr 3 2017, 6:11 PM

This started making the libc++ bots fail, and I'm not convinced the case is reasonable to warn on. That case is:

struct path {
  using value_type = char;
  struct iterator { 
    using value_type = path;
  };
};

Obviously both typedefs are necessary, and according to the standard they are both required to have the same name. I would prefer not to have to #pragma disable this diagnostic within <filesystem>

ahmedasadi updated this revision to Diff 94007.Apr 3 2017, 10:55 PM

Updated diff to address Eric's comment - it's best not to issue a warning if the shadowing declaration is part of a class (this is what GCC currently does).

I will need someone to commit this new patch for me.

this LGTM. thanks for the update.