Diagnose ref-qualifiers occuring after virt-specifier-seq and generate fixit hints

[• ehsan]

Summary

Follow-up to the fix of PR22075.

[rsmith]

Looks good, thanks!

lib/Parse/ParseDecl.cpp

5319–5321 ↗

(On Diff #18291)

Remove braces here, please.

lib/Parse/ParseDeclCXX.cpp

2100–2139 ↗

(On Diff #18291)

Maybe only produce a single diagnostic no matter how many specifiers were provided? You can then also use FixItHint::CreateInsertionFromRange to produce a fixit that copies the complete range of specifiers from after the virt-specifier to before (this will preserve things like macro names and whitespace between the tokens).

[• ehsan]

lib/Parse/ParseDeclCXX.cpp

2100–2139 ↗

(On Diff #18291)

Can you please tell me how that would work? My fixit hints are careful so that if you have something like: void foo() override final &; you would get void foo() final & override; whereas if you have void foo() final override final &; you would get void foo() final & override; (IOW you won't get two final keywords.)

I don't know how to do the same with a range based insertion fixit hint...

cfe/trunk/include/clang/Parse/Parser.h

void ParseTypeQualifierListOpt(DeclSpec &DS,
                                  bool IdentifierRequired = false);
   void ParseDirectDeclarator(Declarator &D);
   void ParseParenDeclarator(Declarator &D);
   void ParseFunctionDeclarator(Declarator &D,
                                ParsedAttributes &attrs,
                                BalancedDelimiterTracker &Tracker,
                                bool IsAmbiguous,
                                bool RequiresArg = false);
+  bool ParseRefQualifier(bool &RefQualifierIsLValueRef,
+                         SourceLocation &RefQualifierLoc);
   bool isFunctionDeclaratorIdentifierList();
   void ParseFunctionDeclaratorIdentifierList(
          Declarator &D,
          SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo);
   void ParseParameterDeclarationClause(
          Declarator &D,
          ParsedAttributes &attrs,
          SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo,

cfe/trunk/lib/Parse/ParseDecl.cpp

if (getLangOpts().CPlusPlus) {
       if (!DS.getSourceRange().getEnd().isInvalid()) {
         EndLoc = DS.getSourceRange().getEnd();
         ConstQualifierLoc = DS.getConstSpecLoc();
         VolatileQualifierLoc = DS.getVolatileSpecLoc();
         RestrictQualifierLoc = DS.getRestrictSpecLoc();
       }
 
       // Parse ref-qualifier[opt].
-      if (Tok.is(tok::amp) || Tok.is(tok::ampamp)) {
-        Diag(Tok, getLangOpts().CPlusPlus11 ?
-             diag::warn_cxx98_compat_ref_qualifier :
-             diag::ext_ref_qualifier);
-
-        RefQualifierIsLValueRef = Tok.is(tok::amp);
-        RefQualifierLoc = ConsumeToken();
+      if (ParseRefQualifier(RefQualifierIsLValueRef, RefQualifierLoc))
         EndLoc = RefQualifierLoc;
-      }
 
       // C++11 [expr.prim.general]p3:
       //   If a declaration declares a member function or member function
       //   template of a class X, the expression this is a prvalue of type
       //   "pointer to cv-qualifier-seq X" between the optional cv-qualifer-seq
       //   and the end of the function-definition, member-declarator, or
       //   declarator.
       // FIXME: currently, "static" case isn't handled correctly.
D.AddTypeInfo(DeclaratorChunk::getFunction(HasProto,
                                              NoexceptExpr.isUsable() ?
                                                NoexceptExpr.get() : nullptr,
                                              ExceptionSpecTokens,
                                              StartLoc, LocalEndLoc, D,
                                              TrailingReturnType),
                 FnAttrs, EndLoc);
 }
 
+/// ParseRefQualifier - Parses a member function ref-qualifier. Returns
+/// true if a ref-qualifier is found.
+bool Parser::ParseRefQualifier(bool &RefQualifierIsLValueRef,
+                               SourceLocation &RefQualifierLoc) {
+  if (Tok.is(tok::amp) || Tok.is(tok::ampamp)) {
+    Diag(Tok, getLangOpts().CPlusPlus11 ?
+         diag::warn_cxx98_compat_ref_qualifier :
+         diag::ext_ref_qualifier);
+
+    RefQualifierIsLValueRef = Tok.is(tok::amp);
+    RefQualifierLoc = ConsumeToken();
+    return true;
+  }
+  return false;
+}
+
 /// isFunctionDeclaratorIdentifierList - This parameter list may have an
 /// identifier list form for a K&R-style function:  void foo(a,b,c)
 ///
 /// Note that identifier-lists are only allowed for normal declarators, not for
 /// abstract-declarators.
 bool Parser::isFunctionDeclaratorIdentifierList() {
   return !getLangOpts().CPlusPlus
          && Tok.is(tok::identifier)

cfe/trunk/lib/Parse/ParseDeclCXX.cpp

void Parser::MaybeParseAndDiagnoseDeclSpecAfterCXX11VirtSpecifierSeq(
     VirtSpecifiers &VS) {
   DeclSpec DS(AttrFactory);
 
   // GNU-style and C++11 attributes are not allowed here, but they will be
   // handled by the caller.  Diagnose everything else.
   ParseTypeQualifierListOpt(DS, AR_NoAttributesParsed, false);
   D.ExtendWithDeclSpec(DS);
 
+  auto &Function = D.getFunctionTypeInfo();
   if (DS.getTypeQualifiers() != DeclSpec::TQ_unspecified) {
     auto DeclSpecCheck = [&] (DeclSpec::TQ TypeQual,
                               const char *FixItName,
                               SourceLocation SpecLoc,
                               unsigned* QualifierLoc) {
       FixItHint Insertion;
-      auto &Function = D.getFunctionTypeInfo();
       if (DS.getTypeQualifiers() & TypeQual) {
         if (!(Function.TypeQuals & TypeQual)) {
           std::string Name(FixItName);
           Name += " ";
           Insertion = FixItHint::CreateInsertion(VS.getFirstLocation(), Name.c_str());
           Function.TypeQuals |= TypeQual;
           *QualifierLoc = SpecLoc.getRawEncoding();
         }
         Diag(SpecLoc, diag::err_declspec_after_virtspec)
           << FixItName
           << VirtSpecifiers::getSpecifierName(VS.getLastSpecifier())
           << FixItHint::CreateRemoval(SpecLoc)
           << Insertion;
       }
     };
-    auto &Function = D.getFunctionTypeInfo();
     DeclSpecCheck(DeclSpec::TQ_const, "const", DS.getConstSpecLoc(),
                   &Function.ConstQualifierLoc);
     DeclSpecCheck(DeclSpec::TQ_volatile, "volatile", DS.getVolatileSpecLoc(),
                   &Function.VolatileQualifierLoc);
     DeclSpecCheck(DeclSpec::TQ_restrict, "restrict", DS.getRestrictSpecLoc(),
                   &Function.RestrictQualifierLoc);
   }
+
+  // Parse ref-qualifiers.
+  bool RefQualifierIsLValueRef = true;
+  SourceLocation RefQualifierLoc;
+  if (ParseRefQualifier(RefQualifierIsLValueRef, RefQualifierLoc)) {
+    const char *Name = (RefQualifierIsLValueRef ? "& " : "&& ");
+    FixItHint Insertion = FixItHint::CreateInsertion(VS.getFirstLocation(), Name);
+    Function.RefQualifierIsLValueRef = RefQualifierIsLValueRef;
+    Function.RefQualifierLoc = RefQualifierLoc.getRawEncoding();
+
+    Diag(RefQualifierLoc, diag::err_declspec_after_virtspec)
+      << (RefQualifierIsLValueRef ? "&" : "&&")
+      << VirtSpecifiers::getSpecifierName(VS.getLastSpecifier())
+      << FixItHint::CreateRemoval(RefQualifierLoc)
+      << Insertion;
+    D.SetRangeEnd(RefQualifierLoc);
+  }
 }
 
 /// ParseCXXClassMemberDeclaration - Parse a C++ class member declaration.
 ///
 ///       member-declaration:
 ///         decl-specifier-seq[opt] member-declarator-list[opt] ';'
 ///         function-definition ';'[opt]
 ///         ::[opt] nested-name-specifier template[opt] unqualified-id ';'[TODO]

cfe/trunk/test/FixIt/fixit-cxx0x.cpp

namespace MisplacedParameterPack {
 
   template <int N...> // expected-error {{'...' must immediately precede declared identifier}}
   void misplacedEllipsisInNonTypeTemplateParameter();
 
   template <int... N...> // expected-error {{'...' must immediately precede declared identifier}}
   void redundantEllipsisInNonTypeTemplateParameter();
 }
 
-namespace MisplacedDeclSpecAfterVirtSpec {
+namespace MisplacedDeclAndRefSpecAfterVirtSpec {
   struct B {
     virtual void f();
     virtual void f() volatile const;
   };
   struct D : B {
     virtual void f() override;
     virtual void f() override final const volatile; // expected-error {{'const' qualifier may not appear after the virtual specifier 'final'}} expected-error {{'volatile' qualifier may not appear after the virtual specifier 'final'}}
   };
+  struct B2 {
+    virtual void f() &;
+    virtual void f() volatile const &&;
+  };
+  struct D2 : B2 {
+    virtual void f() override &; // expected-error {{'&' qualifier may not appear after the virtual specifier 'override'}}
+    virtual void f() override final const volatile &&; //  expected-error {{'const' qualifier may not appear after the virtual specifier 'final'}} expected-error {{'volatile' qualifier may not appear after the virtual specifier 'final'}} expected-error {{'&&' qualifier may not appear after the virtual specifier 'final'}}
+  };
 }