Page MenuHomePhabricator

Improve -Wshadow warnings with enumerators
ClosedPublic

Authored by aaron.ballman on Sep 22 2018, 8:58 PM.

Details

Summary

Currently, we do not check for enumerators that shadow other enumerators as part of -Wshadow, but gcc does provide such a diagnostic for this case. This is intended to catch shadowing issues like:

enum E1{e1};
void f(void) {
  enum E2{e1};
}

This patch addresses PR24718.

Diff Detail

Event Timeline

aaron.ballman created this revision.Sep 22 2018, 8:58 PM
erik.pilkington added inline comments.
lib/Sema/SemaDecl.cpp
16320

I don't think we should do this for scoped enums in C++, i.e. this should be fine:

int Foo;
enum class X { Foo };

Can you enclose this in if (!TheEnumDecl->isScoped()) and add a test? Other than that, LGTM!

aaron.ballman marked an inline comment as done.

Updating based on review feedback.

lib/Sema/SemaDecl.cpp
16320

Thanks, that's a great point! I've corrected in an updated patch.

aaron.ballman accepted this revision.Oct 11 2018, 9:43 AM

Committed in r344259 on @erik.pilkington 's LGTM. Self-accepting so I can close the review.

This revision is now accepted and ready to land.Oct 11 2018, 9:43 AM
aaron.ballman closed this revision.Oct 11 2018, 9:44 AM
sberg added a subscriber: sberg.Oct 16 2018, 3:38 AM

doesnt this make -Wshadow more aggressive for enumerators than for other entities?

~ cat test17.cc
struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };
};

~ llvm/inst/bin/clang++ -fsyntax-only -Wshadow test17.cc 
test17.cc:5:10: warning: declaration shadows a variable in the global namespace
      [-Wshadow]
  enum { S2 };
         ^
test17.cc:2:8: note: previous declaration is here
struct S2;
       ^
1 warning generated.

warns about enumerator S2 but not about similar function S1

(ran into such a new -Wshadow while compiling LibreOffice)

doesnt this make -Wshadow more aggressive for enumerators than for other entities?

It does, but whether that's an issue with the enumerator shadow diagnosing or with the other entities not diagnosing, I'm less clear. Consider this slight modification to your code:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(S2 s);
};

On the one hand, the warning is telling you about a problem before you hit it. However, this code will err on the declaration of S::f() anyway because S2 is not a type, so the warning doesn't get us all *that* much benefit.

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

(ran into such a new -Wshadow while compiling LibreOffice)

Was it a frequent/annoying occurrence?

sberg added a comment.Oct 17 2018, 2:41 AM

[...]

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

but decltype(S2) is a syntax error when S2 names a struct type, no?

(ran into such a new -Wshadow while compiling LibreOffice)

Was it a frequent/annoying occurrence?

there was less than 20 cases overall. about half were "good" warnings about clashing enumerators from different non-scoped enums. the others were "unhelpful" ones about clashes with class names, two of them in stable interface code that cant be changed (so would need ugly #pragma clang warning decorations), one of them even about entities in unrelated include files, so whether a warning is emitted depends on the order in which the files happen to get included in a TU

(and in any case, "declaration shadows a variable" sounds wrong when the shadowed entity is a class type. thats why I thought something is not quite right with this new code yet)

[...]

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

but decltype(S2) is a syntax error when S2 names a struct type, no?

Ugh, you're absolutely right.

(ran into such a new -Wshadow while compiling LibreOffice)

Was it a frequent/annoying occurrence?

there was less than 20 cases overall. about half were "good" warnings about clashing enumerators from different non-scoped enums. the others were "unhelpful" ones about clashes with class names, two of them in stable interface code that cant be changed (so would need ugly #pragma clang warning decorations), one of them even about entities in unrelated include files, so whether a warning is emitted depends on the order in which the files happen to get included in a TU

Thanks, that's good information!

(and in any case, "declaration shadows a variable" sounds wrong when the shadowed entity is a class type. thats why I thought something is not quite right with this new code yet)

Definitely agreed. I think I'm convinced that this should be silenced when the conflict is with a type rather than a value. I'll try to look into this next week when I'm back from WG14 meetings.

(and in any case, "declaration shadows a variable" sounds wrong when the shadowed entity is a class type. thats why I thought something is not quite right with this new code yet)

Definitely agreed. I think I'm convinced that this should be silenced when the conflict is with a type rather than a value. I'll try to look into this next week when I'm back from WG14 meetings.

I've silenced this scenario in r344898, thank you for raising the issue!

I've silenced this scenario in r344898, thank you for raising the issue!

thanks! works fine for me