[PATCH] Improve diagnostic mistyping ":" for "; " after member function declarations (PR7873)

Clang Frontend
1

Hi,

This is my first patch sent directly to clang upstream, please be gentle :slight_smile:

"git show" generated patch, apply with -p1 in .../tools/clang. Passes
"make clang-test" (I'm using cmake builds).

When trying the same mistype with namespace scope declarations we have
a somewhat better situation, the first diagnostic is confusing the the
next one is pretty clear. I could try to address this case too if it
seems valuable.

$ cat case-7873.cc
class Class {
  int m;
  Class();
  virtual ~Class():
  virtual int Func();
};

int Class::Func():
Class::Class():m(10){}

$ ./bin/clang++ -fsyntax-only case-7873.cc
# Generated by the patch.
case-7873.cc:4:19: error: unexpected ':' in (non-constructor) member function
      declaration
  virtual ~Class():
                  ^
                  ;

# Current diagnostic for namespace scope mistyped declarations.
case-7873.cc:9:15: error: expected '{' or ','
Class::Class():m(10){}
              ^
case-7873.cc:8:18: error: only constructors take base initializers
int Class::Func():
                 ^
3 errors generated.

mistyped-colon-for-semicolon.diff (2.57 KB)

2

Ping?

3

This warning is really nice in the case it handles, but it does terrible things when the mistake is an incorrect constructor name instead of a typo of ‘;’. Consider the following code:

% cat x.cc
struct S {
X() : a(1), b(2) {}
explicit X(int) : a(1), b(2) {}

int a, b;
};

% ./bin/clang x.cc
x.cc:2:7: error: unexpected ‘:’ in (non-constructor) member function declaration
X() : a(1), b(2) {}
^
;
x.cc:2:11: error: expected parameter declarator
X() : a(1), b(2) {}
^
x.cc:2:11: error: expected ‘)’
x.cc:2:10: note: to match this ‘(’
X() : a(1), b(2) {}
^
x.cc:2:9: error: C++ requires a type specifier for all declarations
X() : a(1), b(2) {}
^
x.cc:2:17: error: expected parameter declarator
X() : a(1), b(2) {}
^
x.cc:2:17: error: expected ‘)’
x.cc:2:16: note: to match this ‘(’
X() : a(1), b(2) {}
^
x.cc:2:20: error: generalized initializer lists are a C++0x extension unsupported in Clang
X() : a(1), b(2) {}
^
x.cc:2:15: error: C++ requires a type specifier for all declarations
X() : a(1), b(2) {}
~ ^
x.cc:3:29: error: expected parameter declarator
explicit X(int) : a(1), b(2) {}
^
x.cc:3:29: error: expected ‘)’
x.cc:3:28: note: to match this ‘(’
explicit X(int) : a(1), b(2) {}
^
x.cc:3:32: error: generalized initializer lists are a C++0x extension unsupported in Clang
explicit X(int) : a(1), b(2) {}
^
x.cc:3:27: error: C++ requires a type specifier for all declarations
explicit X(int) : a(1), b(2) {}
^
x.cc:5:10: error: C++ requires a type specifier for all declarations
int a, b;
^
x.cc:5:10: error: duplicate member ‘b’
x.cc:3:27: note: previous declaration is here
explicit X(int) : a(1), b(2) {}
^
14 errors generated.

I think you need to look at the tokens following the ‘:’ and try to determine whether it is followed by an initializer list or a declaration. We already have some logic for this in the parser in order to diagnose a missing ‘;’ after ‘struct S {}’. It’s likely re-usable here.

Also, I’d change the text of the message. If you are confident that it is followed by a declaration, and thus should be recovered as a ‘;’ maybe:

“invalid ‘:’ following a non-constructor member declaration, did you mean ‘;’?”

If it might be an initializer list, I would suggest:

“‘:’ implies a constructor initializer, but ‘foo’ is not a constructor”

I’m not convinced we should issue a FixIt hint for the latter case to correct the name to the constructor. Maybe compute the edit distance and look to see if there are any non-constructor aspects to the declaration? (IE, fixit hint to fix the name if the edit distance is below N, there is no return type or specifiers which aren’t allowed on a constructor).