gcc -Wshadow is too strict?

Question

In the following example:

class A
{
  public:
    int len();
    void setLen(int len) { len_ = len; } // warning at this line
  private:
    int len_;
};

gcc with -Wshadow issue a warning:

main.cpp:4: warning: declaration of `len' shadows a member of `this'

function len and integer len are of different type. Why the warning?

Update

I see there's a wide consensus of what "shadow" means. And from a formal point of view, the compiler does exactly what it's meant to do.

However IMHO, the flag is not practical. For example commonly used setter/getter idiom :

class A {
   void prop(int prop);  // setter
   int prop() const;     // getter

   int prop;
};

It would be nice if there was a warning flag that won't issue warning in the case, but will warn in case "int a" hides "int a".

Adding -Wshadow on my legacy code issues tons of warnings, while from time to time I discover bugs caused by "shadowing" problem.

I don't mind how it will be called "-Wmuch_more_practical_and_interesting_shadow" or "-Wfoooooo".

So, are there other gcc warning flags that do what I described?

Update 2

I'm not the only one who thinks -Wshadow is somehow not useful link text. I'm not alone :) Less strict checking could be much more useful.

Answer 1

This seems to be solved on newer versions of GCC.

From version 4.8 changelog:

The option -Wshadow no longer warns if a declaration shadows a function declaration,
unless the former declares a function or pointer to function, because this is a common
and valid case in real-world code.

And it references Linus Torvalds's thoughts on the subject: https://lkml.org/lkml/2006/11/28/253

Unfortunately the newest compiler of the embedded system where I'm currently working is still based on gcc 4.6.

Answer 2

The fact that the parameter has a different type from the member function doesn't affect the fact that the parameter shadows the member function.

Why would you expect there not to be a warning about this?

Answer 3

I don't understand why you insist only on some specific types of shadowing. Shadowing is shadowing, and the dangers of it are the same even if the types are different and even if a variable shadows a function, as in your case. The danger of shadowing is that the code might do something different from what its author wanted it to do.

This, BTW, can easily happen when a variable is shadowing a function, since in C++ the distinction between the two is much thinner than it might seem at the first sight.

For example, here a variable shadows a function

struct L { 
  void operator ()(); 
};

struct A {
  void len();

  A(L len) {
    len();
    // Intended to call the member function. Instead got a call to the functor
  }
};

and I think it is pretty obvious that because of the shadowing the code might do something the author did not intend it to do.

Answer 4

It does exactly what it says on the box. It warns you of shadowing.

Inside the setLen function, two symbols are in scope which share the same name. len is the name of a function parameter, and also the name of a function.

One shadows the name of the other, so when you write code that refers to len, you may not get the result you wanted. Since you asked the compiler to warn you about symbols shadowing for each others, this is what it warns you about.

Answer 5

Even though this is a fairly old question, in GCC >= 7 there now exist three variants of -Wshadow with varying impact on the programmers sanity. From the man-page of GCC 9.1.0:

• -Wshadow=global

The default for -Wshadow. Warns for any (global) shadowing.

• -Wshadow=local

Warn when a local variable shadows another local variable or parameter. This warning is enabled by -Wshadow=global.

• -Wshadow=compatible-local

Warn when a local variable shadows another local variable or parameter whose type is compatible with that of the shadowing variable. In C++, type compatibility here means the type of the shadowing variable can be converted to that of the shadowed variable. The creation of this flag (in addition to -Wshadow=local) is based on the idea that when a local variable shadows another one of incompatible type, it is most likely intentional, not a bug or typo, as shown in the following example:

               for (SomeIterator i = SomeObj.begin(); i != SomeObj.end(); ++i)
               {
                 for (int i = 0; i < N; ++i)
                 {
                   ...
                 }
                 ...
               }

Since the two variable "i" in the example above have incompatible types, enabling only -Wshadow=compatible-local will not emit a warning. Because their types are incompatible, if a programmer accidentally uses one in place of the other, type checking will catch that and emit an error or warning. So not warning (about shadowing) in this case will not lead to undetected bugs. Use of this flag instead of -Wshadow=local can possibly reduce the number of warnings triggered by intentional shadowing.

This warning is enabled by -Wshadow=local.

Answer 6

Yea, we can tune this warning to warn only when the shadowing might be dangerous. For example do not shadow for

void set(int what).. int what() const ..

but warn for local variable shadowning and for the functor example above.

To be more exact, warn when the shadowing might be dangerous, when the intent of the code writer might not be clear. In the case of an int parameter and a member function having the same name, it is clear that the author does not want the member to be called when he/she references the parameter.

I find this shadow warning to be a good idea, very helpful, it just needs a bit more thought to not warn about perfectly safe and clear cases. I for example can live with arguments needed to have a prefix or something, but I prefer clean and simple names.

Answer 7

From the Update section of the question:

class A {
   void prop(int prop);  // setter
   int prop() const;     // getter

   int prop;
};

This is simply an error; you cannot have both a member variable and a method with the same name.

The minimal changes with respect to your example that give a working interface are:

class A {
public:
  void prop(int prop);  // setter
  int prop() const;     // getter

private:
   int prop_;
};