-Wshadow rationale for data members

Clang Frontend
1

Hi,

struct A {
   static void* f; void* g;
   struct B { static void* f; void* g; };
};
struct C { static void* f; void* g;};
struct D: public C { static void* f; void* g; };

compiled with
clang++ -c -Wall -Wshadow -O2 t.cxx
generates the warning

t.cxx:3:28: warning: declaration shadows a static data member of 'A'
[-Wshadow]
   struct B { static void* f; void* g; };
                           ^
t.cxx:2:17: note: previous declaration is here
   static void* f; void* g;
                ^

What about non-static members? And why only for nested classes (B), not
for inheritance (D)? Shouldn't nested classes be allowed to re-define
static variables with the same name as an outer one?

Cheers, Axel.

2

It doesn't seem at all useful for Clang to warn about static data members shadowing something else by the same name. Personally, I'd limit -Wshadow to only complain when a local declaration shadows something from the outer scope.

  - Doug

3

I agree.

John.

4

Hi,

5

Hi,

clang_Wshadow_locals.diff (1.37 KB)

6

Yeah, I guess that's what we're looking for. Please lift the comment up into the block comment above the if statement, though.

John.

7

Hi,

struct A {
static void* f; void* g;
struct B { static void* f; void* g; };
};
struct C { static void* f; void* g;};
struct D: public C { static void* f; void* g; };

compiled with
clang++ -c -Wall -Wshadow -O2 t.cxx
generates the warning

t.cxx:3:28: warning: declaration shadows a static data member of 'A'
[-Wshadow]
struct B { static void* f; void* g; };
                       ^
t.cxx:2:17: note: previous declaration is here
static void* f; void* g;
            ^

It doesn't seem at all useful for Clang to warn about static data members shadowing something else by the same name. Personally, I'd limit -Wshadow to only complain when a local declaration shadows something from the outer scope.

I agree.

You mean like in the attached patch? The corresponding tests still to be
updated (removed, mostly), i.e. please don't commit :slight_smile:

Incidentally, I have just added a comment to bug 6808 (other false -Wshadow warnings) yesterday.

I'm attaching a patch that combines the above changes (as Axel just removed a few things) and also prevents false warnings (1), (2), (3) in the following snippet:

class A{
    int a;
    static void f(int a){ } // (1) false warning (in contrast to gcc)
    class X{
        X(int a){ } // (2) false warning (in contrast to gcc)
        static void g(int a){ } // (3) false warning (in contrast to gcc)
    };
};

I'm new to Clang, so could you please doublecheck the code and commit it for me, if it's OK?

I also wonder whether (5), (6), and (7) in this code should produce a warning:

class X{
    int a;
    class X{
        enum{ a = 0 }; // (4) no warning (OK)
        X(int a){ } // (5) missing warning? (in contrast to gcc)
        static void g(int a){ } // (6) missing warning? (like gcc)
    };
};

class Y{
    static int a;
    class X{
        enum{ a = 0 }; // (7) missing warning?
    };
};

clang_Wshadow_staticmeths_and_ctors.diff (2.29 KB)

8

I extended the previous patch to cases when a field (as opposed to static member) of class A would be "shadowed" by any variable occurring in a class not derived from A, like in this example:

struct A{
  int a;
  struct X{
    void f(int a){}
  };
};

(Also, the lines in the patch do not exceed 80 columns anymore.)

This probably prevents most false warnings. Any comments on the cases I mentioned in the previous message?

clang_Wshadow_staticmeths_and_notderivedfrom.diff (2.34 KB)

9

I'm attaching a patch that combines the above changes (as Axel just removed a few things) and also prevents false warnings (1), (2), (3) in the following snippet:

class A{
   int a;
   static void f(int a){ } // (1) false warning (in contrast to gcc)
   class X{
       X(int a){ } // (2) false warning (in contrast to gcc)
       static void g(int a){ } // (3) false warning (in contrast to gcc)
   };
};

I'm new to Clang, so could you please doublecheck the code and commit it for me, if it's OK?

(reviewing your new patch)

If you're in a CXXMethodDecl, you're in a C++ translation unit, so all RecordDecls will be CXXRecordDecls; you don't need to test for that. In fact, we should never warn about shadowing a FieldDecl except in C++.

The preferred way to get the innermost function is getCurFunctionOrMethodDecl(), or just getCurFunction() if you don't care about being in an ObjC method.

You could avoid a lot of these cast<>s if you used dyn_cast instead of isa, as in:
  if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FnDC))

Otherwise this looks fine unless you want to revise your patch as below. Also, please include your test cases as part of your next patch.

I also wonder whether (5), (6), and (7) in this code should produce a warning:

class X{
   int a;
   class X{
       enum{ a = 0 }; // (4) no warning (OK)

This shouldn't warn. The rules I have in mind are:
- we shouldn't warn about shadowing something that wasn't valid to reference in the first place, e.g. a local variable from a function we're not in; otherwise
- we should warn about shadowing something that can no longer be referenced due to the shadowing, i.e. a local variable; otherwise
- we should warn about shadowing fields from base classes with other fields, or data members with local variables; otherwise
- we shouldn't warn about shadowing class members, or when class members shadow non-class members; otherwise
- we should warn.

Do those seem like reasonable principles to start with?

       X(int a){ } // (5) missing warning? (in contrast to gcc)

This definitely shouldn't warn.

       static void g(int a){ } // (6) missing warning? (like gcc)

Neither should this.

   };
};

class Y{
   static int a;
   class X{
       enum{ a = 0 }; // (7) missing warning?

Nor should this.

John.

10

John,

Thank you very much for a thorough review!

No problem. Please reply to cfe-dev, though, just so other people can chip in.

If you're in a CXXMethodDecl, you're in a C++ translation unit, so all RecordDecls will be CXXRecordDecls; [...]

Thanks for clarification, wasn't sure about that.

The preferred way to get the innermost function is getCurFunctionOrMethodDecl(), [...]

OK.

You could avoid a lot of these cast<>s if you used dyn_cast instead of isa, [...]

OK.

Also, please include your test cases as part of your next patch.

OK.

As for the decision what should be warned about, I will think it once over and possibly rewrite the code.

I mostly agreee with what you have written, but here's a few questions:

- we shouldn't warn about shadowing class members, or when class members shadow non-class members; otherwise

Why not? Is the rationale that if needed you can still access class members by additional qualification?(You could apply that to base class instance variables too.)

Well, we definitely need to be more careful about warning about shadowing class members. Often you get class member functions with very generic names that aren't necessarily a problem to shadow. But I'd be fine with you experimenting with finer-grained rules here.

I do think we definitely want to not warn about shadowing outer class members with inner class members, though.

class X{
int a;
class X{
     enum{ a = 0 }; // (4) no warning (OK)
     X(int a){ } // (5) missing warning? (in contrast to gcc)

This definitely shouldn't warn.

Why not? Would you warn if "a" was a "const int", not an enum constant?

I think I missed that 'a' was shadowing the enum, not the outer member. This is okay to warn about.

John.

11

Hello all,

I decided to do a more thorough revision of what -Wshadow so that its behavior makes more sense. Any comments on the following are welcome:

(Note: By "shadow" in the folliwing I mean "be reported to shadow".)

1. What can currently shadow what

12

I decided to do a more thorough revision of what -Wshadow so that its behavior makes more sense. Any comments on the following are welcome:

(Note: By "shadow" in the folliwing I mean "be reported to shadow".)

1. What can currently shadow what

1.1. Warnings are issued for variable declarations (VarDecl) shadowing some other variables (local or global), fields, and static data members.

Okay.

1.2. There are some false warnings that can be eliminated easily (the patch I have posted earlier in this thread).

1.3. The comment above CheckShadow() says "Diagnose variable or built-in function shadowing", which seems no longer to be the case. (I don't know what the original code was like, but now redeclaration of a builtin triggers an error, and it is not handled by CheckShadow().) The comment is outdated, right?

Yes.

2. What currently cannot shadow or be shadowed

2.1. A type declaration cannot shadow or be shadowed. OK?

2.2. A function cannot shadow or be shadowed. OK?

2.3. A namespace cannot shadow or be shadowed. OK?

These all seem reasonable.

2.4. Fields or static data members cannot shadow. This is worth considering: Do you agree that shadowing an accessible field of a base class with another field is worth reporting?

Yes, this is definitely a goal of -Wshadow.

What about static data members: I think that only a static data member shadowing a member variable from a base class is worth considering. Do you agree?

Yes.

2.5. An enum constant cannot shadow or be shadowed. Do you agree that enum constants should be treated similarly to variables or, within a class/structure context, to static data members?

I think we shouldn't warn about shadowing enumerators.

John.

13

2.5. An enum constant cannot shadow or be shadowed. Do you agree that enum constants should be treated similarly to variables or, within a class/structure context, to static data members?

I think we shouldn't warn about shadowing enumerators.

I'm not sure whether we mean the same thing. I mean "a" (enum constant), not "A" (enum tag), in "enum A{a};".

Enum constants are little different from variables/static data variables: they can be defined at the same places, have the same scoping rules, and can mostly be used in the same syntactic context (of course there's no storage associated with enum constants).

I can't see why there should be any difference between how x's and y's are handled in the following:

const int x = 0;
enum{ y = 0 };
void f(){
  const int x = 1;
  enum{ y = 0 };
  // ^ or vice versa: const int y, enum{x}
}

struct A{
  const int x = 0;
  enum{ y = 0 };
};
struct B : public A{
  static const int x = 1;
  enum{ y = 0 };
  // ^ or vice versa: const int y, enum{x}
};

14

2.5. An enum constant cannot shadow or be shadowed. Do you agree that enum constants should be treated similarly to variables or, within a class/structure context, to static data members?

I think we shouldn't warn about shadowing enumerators.

I'm not sure whether we mean the same thing. I mean "a" (enum constant), not "A" (enum tag), in "enum A{a};".

Yes. The constants are sometimes also called enumerators; sorry for being unclear.

Enum constants are little different from variables/static data variables: they can be defined at the same places, have the same scoping rules, and can mostly be used in the same syntactic context (of course there's no storage associated with enum constants).

I can't see why there should be any difference between how x's and y's are handled in the following:

I am primarily worried about flooding the world with -Wshadow warnings that we, and gcc, have never emitted before. On the other hand, I don't personally enable -Wshadow, and it's not part of -Wall, and I can't deny the inconsistency, so if you think that -Wshadow should warn about shadowing enumerators (treating them as static variables when they're class members) then we can try that experiment.

John.

15

Enum constants are little different from variables/static data variables: they can be defined at the same places, have the same scoping rules, and can mostly be used in the same syntactic context (of course there's no storage associated with enum constants).

I can't see why there should be any difference between how x's and y's are handled in the following:

I am primarily worried about flooding the world with -Wshadow warnings that we, and gcc, have never emitted before.

Yes, it certainly shouldn't be overdone. One bad example is reporting shadowed functions. Thanks to overzealous -Wshadow in some old release of GCC I know that there's an index() function somewhere in the standard C headers;).

On the other hand, I don't personally enable -Wshadow, and it's not part of -Wall, and I can't deny the inconsistency, so if you think that -Wshadow should warn about shadowing enumerators (treating them as static variables when they're class members) then we can try that experiment.

A small supportive argument is that GCC already takes enums into account (although not consistently). It does so at least since version 4.0 and I haven't been able to find any complaints on the web.

Incidentally I also found this in GCC 4.6 release notes: "The -Wshadow option now warns if a local variable or type declaration shadows another type in C++. Note that the compiler will not warn if a local variable shadows a struct/class/enum, but will warn if it shadows an explicit typedef."

So I guess that we're still being quite conservative. Anyway, I'll try doing the non-enum stuff first and wait if there are any more comments on this.

Have a nice weekend,