[cfe-dev] Patch: Re: -Wshadow rationale for data members

[John McCall]

On Oct 30, 2010, at 4:25 AM, Adam Nohejl wrote:
> 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.