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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Sema/DeclSpec.h:1365
+        AttributeFactory AttrFactory;
+        MethodQualifiers = new DeclSpec(AttrFactory);
+      }
----------------
This `DeclSpec` ends up with a dangling reference to the `AttributeFactory`; I think you need to allocate them both when used.

Consider having this return a `DeclSpec &` and making it the primary way that clients get a mutable DS.


================
Comment at: include/clang/Sema/DeclSpec.h:1405
     SourceLocation getConstQualifierLoc() const {
-      return SourceLocation::getFromRawEncoding(ConstQualifierLoc);
+      return MethodQualifiers->getConstSpecLoc();
     }
----------------
The "if any" suggests that that this method (and below) can be called on a DS that doesn't have this qualifier.  If that's not actually true, you should assert that `MethodQualifiers` exists and change the comment.


================
Comment at: include/clang/Sema/DeclSpec.h:1442
+             (MethodQualifiers->getTypeQualifiers() & Qualifiers::Restrict);
+    }
+
----------------
It might be reasonable to just remove these accessors and make more things query the DS if it exists.


================
Comment at: include/clang/Sema/DeclSpec.h:1606
                                      Declarator &TheDeclarator,
+                                     DeclSpec& MethodQualifiers,
                                      TypeResult TrailingReturnType =
----------------
Spacing


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2352
         FixItHint Insertion;
         if (DS.getTypeQualifiers() & TypeQual) {
+          if (!(Function.MethodQualifiers &&
----------------
Since you're touching this code anyway, please make this an early return (before the declaration of `Insertion`) and de-indent the rest of the helper.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2354
+          if (!(Function.MethodQualifiers &&
+                (Function.MethodQualifiers->getTypeQualifiers() & TypeQual))) {
             std::string Name(FixItName);
----------------
Either `MethodQualifiers` already exists (because it has the qualifier) or you'll be creating it (to add the qualifier); might as well force it to exist eagerly and simplify the logic here and below.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2368
         }
       };
+      DeclSpecCheck(DeclSpec::TQ_const, "const", DS.getConstSpecLoc());
----------------
Please remove this dead semicolon since you're touching this code anyway.


================
Comment at: lib/Sema/DeclSpec.cpp:175
                                              Declarator &TheDeclarator,
+                                             DeclSpec& MethodQualifiers,
                                              TypeResult TrailingReturnType) {
----------------
Consider taking this as a pointer just so that the various places that construct one of these when method qualifiers are impossible don't have to construct a DS unnecessarily.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8374
       Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_destructor)
-        << "restrict" << SourceRange(D.getIdentifierLoc());
+          << "restrict" << SourceRange(D.getIdentifierLoc());
     D.setInvalidType();
----------------
Should there be a method on DS that iterates over the type qualifiers present?  You could have it take an `llvm::function_ref<void(TQ, StringRef, SourceLoc)>` or something.


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

https://reviews.llvm.org/D55948