[Parse] Improve diagnostic and recoveryy when there is an extra override in the outline method definition.

[hokein]

Summary

The clang behavior was poor before this patch:

void B::foo() override {}
// Before: clang emited "expcted function body after function
// declarator", and skiped all contents until it hits a ";", the
// following function f() is discarded.

// VS

// Nnow "override is not allowed" with a remove fixit, and following f()
// is retained.
void f();

[sammccall]

This seems very nice to me, I'd hit the bad diagnostic before but hadn't noticed the bad recovery.
AFAICT this can't break any valid code.

clang/include/clang/Basic/DiagnosticParseKinds.td

962

virt-specifier is standardese. I think dropping virt, i.e. 'override' specifier... is clear enough.

962

Hm, is "outside a class definition" the clearest way to explain the mistake?

I guess this is handling two classes of problem:

• repeating override on an out-of-line definition (motivating case)
• placing override on a free function

I'd slightly prefer the motivating case to mention "out-of-line", but that wording doesn't fit the other case. I'm not sure whether it's worth trying to split them

clang/lib/Parse/ParseDecl.cpp

2039

Do we too have to consider K&R?

typedef int override;
void foo(a) 
    override a; 
{
}

I guess not because this behavior only fires for c++?

[hokein]

virt-specifier => specifier

clang/include/clang/Basic/DiagnosticParseKinds.td

962

yeah, the first one is the motivating case, I think it is the most common one, but I'm tempted to keep it as-is rather than creating a separate diagnostic message for a less-common case.

clang/lib/Parse/ParseDecl.cpp

2039

right, the change only affects C++ code -- isCXX11VirtSpecifier returns null for non-C++11-or-later code.

clang/include/clang/Basic/DiagnosticParseKinds.td

def err_access_specifier_interface : Error<
   "interface types cannot specify '%select{private|protected}0' access">;
 
 def err_duplicate_class_virt_specifier : Error<
   "class already marked '%0'">;
 
 def err_duplicate_virt_specifier : Error<
   "class member already marked '%0'">;
 
+def err_virt_specifier_outside_class : Error<
+  "'%0' specifier is not allowed outside a class definition">;
+
 def err_expected_parameter_pack : Error<
   "expected the name of a parameter pack">;
 def err_paren_sizeof_parameter_pack : Error<
   "missing parentheses around the size of parameter pack %0">;
 def err_sizeof_parameter_pack : Error<
   "expected parenthesized parameter pack name in 'sizeof...' expression">;
 
 // C++11 lambda expressions

clang/lib/Parse/ParseDecl.cpp

Parser::DeclGroupPtrTy Parser::ParseDeclGroup(ParsingDeclSpec &DS,
 
   // Check to see if we have a function *definition* which must have a body.
   if (D.isFunctionDeclarator()) {
     if (Tok.is(tok::equal) && NextToken().is(tok::code_completion)) {
       cutOffParsing();
       Actions.CodeCompleteAfterFunctionEquals(D);
       return nullptr;
     }
+    // We're at the point where the parsing of function declarator is finished.
+    //
+    // A common error is that users accidently add a virtual specifier
+    // (e.g. override) in an out-line method definition.
+    // We attempt to recover by stripping all these specifiers coming after
+    // the declarator.
+    while (auto Specifier = isCXX11VirtSpecifier()) {
+      Diag(Tok, diag::err_virt_specifier_outside_class)
+          << VirtSpecifiers::getSpecifierName(Specifier)
+          << FixItHint::CreateRemoval(Tok.getLocation());
+      ConsumeToken();
+    }
     // Look at the next token to make sure that this isn't a function
     // declaration.  We have to check this because __attribute__ might be the
     // start of a function definition in GCC-extended K&R C.
     if (!isDeclarationAfterDeclarator()) {
 
       // Function definitions are only allowed at file scope and in C++ classes.
       // The C++ inline method definition case is handled elsewhere, so we only
       // need to handle the file scope definition case.
       if (Context == DeclaratorContext::File) {
         if (isStartOfFunctionDefinition(D)) {
           if (DS.getStorageClassSpec() == DeclSpec::SCS_typedef) {
             Diag(Tok, diag::err_function_declared_typedef);

clang/test/Parser/cxx-extra-virtual-specifiers.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -fdiagnostics-parseable-fixits %s
+
+class A {
+  virtual void foo();
+};
+class B : public A {
+  void foo() override;
+};
+
+void B::foo() override {} // expected-error {{'override' specifier is not allowed outside a class definition}}
+                          // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:24}:""
+
+void f1() override; // expected-error {{'override' specifier is not allowed}}
+
+void f2() override {} // expected-error {{'override' specifier is not allowed}}
+
+void test() {
+  void f() override; // expected-error {{'override' specifier is not allowed}}
+}