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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits