Treat qualifiers on elaborated types for qualtypenames appropriately.

[saugustine]

Summary

Treat qualifiers on elaborated types for qualtypenames appropriately.

[rsmith]

lib/Tooling/Core/QualTypeNames.cpp

386–390

Qualtype -> QualType

393–394

Maybe add an assert that QT is unqualified here?

401–402

I find the way this code ensures that we preserve the qualifiers to be a little subtle. It's not obvious to me that this does the right thing for a case like

struct X;
void f(const ::X x) {}

... where we have an ElaboratedType with no keyword, and for which we will generate an empty Prefix -- it looks like we would lose the const on line 392 and never add it back.

Can you remove and re-add the qualifiers unconditionally? (That is, move this removal of qualifiers from QT to after line 389, and move line 419 outside the if.) I think that'll make the logic clearer.

409–411

With the change suggested above, you should be able to delete the handling of qualifiers here.

[dblaikie]

unittests/Tooling/QualTypeNamesTest.cpp

91

What does this produce without your change? (what's the change causing to happen?)

[saugustine]

• Handle elaborated types even more cleanly.

[saugustine]

Thanks for the reviews. I believe I have addressed all issues. Take another look when you get the chance.

lib/Tooling/Core/QualTypeNames.cpp

401–402

We can remove and put them back unconditionally, but we still need to condition getting a new elaborated type, because an assertion prevents creating an elaborated type without a prefix of an empty keyword.

I'm not sure what you expect on the new example, but I have added it as a test case (as a pointer because the struct is incomplete). The result we produce in this case is "const X *", which I think is correct.

unittests/Tooling/QualTypeNamesTest.cpp

91

Without the change, we get "A::B::Class0 *" with no "const".

[rsmith]

LGTM with a couple of minor tweaks, thanks!

lib/Tooling/Core/QualTypeNames.cpp

401–402

Please move this up to immediately after line 389, so that we have a single point where we split QT into a type pointer and qualifiers.

401–402

Sure, building the elaborated type should still be conditional on there being one to build :)

const X * looks correct to me for your new example (I think with the old code we'd have got just X *).

407

Maybe pont -> point while you're here :)

411

QT = QualType(TypePtr, 0); would be fine here.

[saugustine]

• Address remaining nits from review.

[saugustine]

Thanks again. I've addressed the last little bits. Mind checking this in for me?

[rsmith]

Committed as rL268988.

lib/Tooling/Core/QualTypeNames.cpp

while (isa<SubstTemplateTypeParmType>(QT.getTypePtr())) {
 
     QT = dyn_cast<SubstTemplateTypeParmType>(QT.getTypePtr())->desugar();
 
     // Add back the qualifiers.
     QT = Ctx.getQualifiedType(QT, Quals);
   }
 
   NestedNameSpecifier *Prefix = nullptr;
-  Qualifiers PrefixQualifiers;
+  // Local qualifiers are attached to the QualType outside of the
+  // elaborated type.  Retrieve them before descending into the
+  // elaborated type.
+  Qualifiers PrefixQualifiers = QT.getLocalQualifiers();
+  QT = QualType(QT.getTypePtr(), 0);
   ElaboratedTypeKeyword Keyword = ETK_None;
   if (const auto *ETypeInput = dyn_cast<ElaboratedType>(QT.getTypePtr())) {
     QT = ETypeInput->getNamedType();
+    assert(!QT.hasLocalQualifiers());
     Keyword = ETypeInput->getKeyword();
   }
   // Create a nested name specifier if needed (i.e. if the decl context
   // is not the global scope.
   Prefix = createNestedNameSpecifierForScopeOf(Ctx, QT.getTypePtr(),
                                                true /*FullyQualified*/);
 
-  // move the qualifiers on the outer type (avoid 'std::const string'!)
-  if (Prefix) {
-    PrefixQualifiers = QT.getLocalQualifiers();
-    QT = QualType(QT.getTypePtr(), 0);
-  }
-
   // In case of template specializations iterate over the arguments and
   // fully qualify them as well.
   if (isa<const TemplateSpecializationType>(QT.getTypePtr()) ||
       isa<const RecordType>(QT.getTypePtr())) {
     // We are asked to fully qualify and we have a Record Type (which
-    // may pont to a template specialization) or Template
+    // may point to a template specialization) or Template
     // Specialization Type. We need to fully qualify their arguments.
 
-    Qualifiers Quals = QT.getLocalQualifiers();
     const Type *TypePtr = getFullyQualifiedTemplateType(Ctx, QT.getTypePtr());
-    QT = Ctx.getQualifiedType(TypePtr, Quals);
+    QT = QualType(TypePtr, 0);
   }
   if (Prefix || Keyword != ETK_None) {
     QT = Ctx.getElaboratedType(Keyword, Prefix, QT);
-    QT = Ctx.getQualifiedType(QT, PrefixQualifiers);
   }
+  QT = Ctx.getQualifiedType(QT, PrefixQualifiers);
   return QT;
 }
 
 std::string getFullyQualifiedName(QualType QT,
                                   const ASTContext &Ctx) {
   PrintingPolicy Policy(Ctx.getPrintingPolicy());
   Policy.SuppressScope = false;
   Policy.AnonymousTagLocations = false;
   Policy.PolishForDeclaration = true;
   Policy.SuppressUnwrittenScope = true;
   QualType FQQT = getFullyQualifiedType(QT, Ctx);
   return FQQT.getAsString(Policy);
 }
 
 }  // end namespace TypeName
 }  // end namespace clang

unittests/Tooling/QualTypeNamesTest.cpp

TEST(QualTypeNameTest, getFullyQualifiedName) {
   // Anonymous Namespaces nested in named namespaces and vice-versa.
   Visitor.ExpectedQualTypeNames["CheckK"] = "D::aStruct";
   // Namespace alias
   Visitor.ExpectedQualTypeNames["CheckL"] = "A::B::C::MyInt";
   Visitor.ExpectedQualTypeNames["non_dependent_type_var"] =
       "Foo<X>::non_dependent_type";
   Visitor.ExpectedQualTypeNames["AnEnumVar"] = "EnumScopeClass::AnEnum";
   Visitor.ExpectedQualTypeNames["AliasTypeVal"] = "A::B::C::InnerAlias<int>";
+  Visitor.ExpectedQualTypeNames["CheckM"] = "const A::B::Class0 *";
+  Visitor.ExpectedQualTypeNames["CheckN"] = "const X *";
   Visitor.runOver(
       "int CheckInt;\n"
       "template <typename T>\n"
       "class OuterTemplateClass { };\n"
       "namespace A {\n"
       " namespace B {\n"
       "   class Class0 { };\n"
       "   namespace C {\n"
       "     typedef int MyInt;"
       "     template <typename T>\n"
       "     using InnerAlias = OuterTemplateClass<T>;\n"
       "     InnerAlias<int> AliasTypeVal;\n"
       "   }\n"
       "   template<class X, class Y> class Template0;"
       "   template<class X, class Y> class Template1;"
       "   typedef B::Class0 AnotherClass;\n"
       "   void Function1(Template0<C::MyInt,\n"
       "                  AnotherClass> CheckC);\n"
       "   void Function2(Template0<Template1<C::MyInt, AnotherClass>,\n"
       "                            Template0<int, long> > CheckD);\n"
+      "   void Function3(const B::Class0* CheckM);\n"
       "  }\n"
       "template<typename... Values> class Variadic {};\n"
       "Variadic<int, B::Template0<int, char>, "
       "         B::Template1<int, long>, "
       "         B::C::MyInt > CheckE;\n"
       " namespace BC = B::C;\n"
       " BC::MyInt CheckL;\n"
       "}\n"
       "using A::B::Class0;\n"
       "void Function(Class0 CheckF);\n"
       "using namespace A::B::C;\n"
       "void Function(MyInt CheckG);\n"
       "void f() {\n"
       "  struct X {} CheckH;\n"
       "}\n"
+      "struct X;\n"
+      "void f(const ::X* CheckN) {}\n"
       "namespace {\n"
       "  class aClass {};\n"
       "   aClass CheckI;\n"
       "}\n"
       "namespace A {\n"
       "  struct aStruct {} CheckJ;\n"
       "}\n"
       "namespace {\n"