Should this code cause a warning

Question

I came across code similar to that below which is legal, albeit not very clever. Two stack variables in the same function in scope at the same time with the same name is legal but it can cause problems. (Read: I just wasted half an hour debugging this). At warning level 4 on VS2010 (the highest level), I would have hoped it would have caught this kind of thing. Am I missing something, or is it time to hit to whole code base with lint for a once over? Would a static analysis tool such as lint even pick up name clashes like this?

   char *x = strchr(Buffer,' ');
   if (x)
   {
     *x = 0;
     x++;
     char *x = strchr(x,' ') 
     if (x)
        *x = 0;
   }

Answer 1

PC-Lint will detect this. The following code:

main (int argc, char *argv []) 
{
    char *x = 0;
    if (x)
    {
        char *x = 0;
    }
}

Gives a warning:

main.cpp  6  Warning 578: Declaration of symbol 'x' hides symbol 'x' (line 3)

Answer 2

Edit: I didn't notice this when I wrote the original answer (below). The code you've posted is illegal, and causes undefined behavior. The offending line is this one:

char *x = strchr(x,' ');

Here the x within the strchr call does not refer to the x defined in the enclosing scope, but to the x defined previously on the same line. Hence that line reads from an uninitialized variable, which results in undefined behavior. From the C++ standard,

§3.3.2/1 [basic.scope.pdecl]
The point of declaration for a name is immediately after its complete declarator (Clause 8) and before its initializer (if any), except as noted below. [ Example:

   int x = 12;
   { int x = x; }

Here the second x is initialized with its own (indeterminate) value. —end example ]

GCC does complain if the corresponding line in the example below is changed to

int x = 21 + x; // generates "warning: x is used uninitialized in this function"

And duplicating your strchr example on VS2012 generates this warning (at /W1 and above):

warning C4700: uninitialized local variable 'x' used

---

Original answer follows (not entirely accurate):

There's nothing illegal about the code. You've introduced a new scope by adding braces, and you're allowed to define variables within the new scope even if those variable names have been previously defined in the enclosing scope.

All references to the variable after the new definition will refer to the local variable instead of the one in the enclosing scope, until the lifetime of the local variable ends. The following code produces no warnings on GCC even when compiled with -pedantic -Wall -Wextra

#include <iostream>

int main()
{
  int x = 42;
  {
      std::cout << x << '\n';
      int x = 21;
      std::cout << x << '\n';
  }
  std::cout << x << '\n';
}

Output:

42
21
42

I don't know whether lint or another static analysis tool will pick out such things either. It is legal, but not advisable.