[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2368
         }
       };
+      DeclSpecCheck(DeclSpec::TQ_const, "const", DS.getConstSpecLoc());
----------------
Anastasia wrote:
> rjmccall wrote:
> > Please remove this dead semicolon since you're touching this code anyway.
> Here the semicolon is at the end of a lambda declaration statement.
Oh, yes it is, sorry.


================
Comment at: lib/Parse/ParseExprCXX.cpp:1264
     SourceLocation NoLoc;
+    DeclSpec NoDS(AttrFactory);
     D.AddTypeInfo(DeclaratorChunk::getFunction(
----------------
This is dead now.


================
Comment at: lib/Sema/DeclSpec.cpp:220
+
+  I.Fun.QualAttrFactory         = nullptr;
+
----------------
Spacing?


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8175
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
-  if (FTI.TypeQuals != 0) {
-    if (FTI.TypeQuals & Qualifiers::Const)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-        << "const" << SourceRange(D.getIdentifierLoc());
-    if (FTI.TypeQuals & Qualifiers::Volatile)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-        << "volatile" << SourceRange(D.getIdentifierLoc());
-    if (FTI.TypeQuals & Qualifiers::Restrict)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-        << "restrict" << SourceRange(D.getIdentifierLoc());
+  if (FTI.MethodQualifiers && FTI.MethodQualifiers->getTypeQualifiers() != 0) {
+    auto DiagQual = [&](DeclSpec::TQ TypeQual, StringRef QualName,
----------------
I think you should add a `hasMethodQualifiers` method to FTI that does this check.  Note that it needs to check for attributes, too, and I think you need to figure out some way to generalize `forEachCVRUQual` to cover those.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8181
+    };
+    FTI.MethodQualifiers->forEachCVRUQual(DiagQual);
     D.setInvalidType();
----------------
I'd write this lambda inline as the argument, but if you think it's better separate, that's okay.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8361
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
-  if (FTI.TypeQuals != 0 && !D.isInvalidType()) {
-    if (FTI.TypeQuals & Qualifiers::Const)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_destructor)
-        << "const" << SourceRange(D.getIdentifierLoc());
-    if (FTI.TypeQuals & Qualifiers::Volatile)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_destructor)
-        << "volatile" << SourceRange(D.getIdentifierLoc());
-    if (FTI.TypeQuals & Qualifiers::Restrict)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_destructor)
-        << "restrict" << SourceRange(D.getIdentifierLoc());
+  if (FTI.MethodQualifiers && FTI.MethodQualifiers->getTypeQualifiers() != 0 &&
+      !D.isInvalidType()) {
----------------
Another place for `hasMethodQualifiers`.


================
Comment at: lib/Sema/SemaType.cpp:719
   SourceLocation NoLoc;
+  AttributeFactory attrFactory;
   declarator.AddInnermostTypeInfo(DeclaratorChunk::getFunction(
----------------
This is dead now.


================
Comment at: lib/Sema/SemaType.cpp:4460
+      IsQualifiedFunction =
+          (FTI.MethodQualifiers && FTI.MethodQualifiers->getTypeQualifiers()) ||
+          FTI.hasRefQualifier();
----------------
This would be a good place to use `hasMethodQualifiers` (unless there's a reason to ignore attribute qualifiers here).


================
Comment at: lib/Sema/SemaType.cpp:5035
+            RemovalLocs.push_back(SL);
+          };
+          Chunk.Fun.MethodQualifiers->forEachCVRUQual(AddToRemovals);
----------------
A lambda this simple should definitely be written inline.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55948/new/

https://reviews.llvm.org/D55948