[PATCH] D15861: Support fully-qualified names for all QualTypes
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 13 12:47:53 PST 2016
rsmith added a comment.
I think this functionality can be provided more simply by adding another flag to `PrintingPolicy` to request fully-qualified names always be produced. It already usually provides fully-qualified names; the only notable exception I can see is that if a qualifier was already provided, it uses that instead (in particular, there is a special case for printing `ElaboratedType`s that uses the qualifier as written).
================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:55
@@ +54,3 @@
+// client using that name at the end of the translation unit will be
+// refering to a different type.
+//
----------------
refering -> referring
================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:80
@@ +79,3 @@
+/// issues such as type shadowing.
+
+/// \param[in] QT - the type for which the fully qualified type will be
----------------
Missing `///` on this line.
================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:107
@@ +106,3 @@
+ const ASTContext &Ctx,
+ const PrintingPolicy& Policy);
+
----------------
` &`, not `& `, please.
================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:118-139
@@ +117,24 @@
+
+/// \brief Create a NestedNameSpecifier for TagDecl and its enclosing
+/// scopes.
+///
+/// \param[in] Ctx - the AST Context to be used.
+/// \param[in] TD - the TagDecl for which a NestedNameSpecifier is
+/// requested.
+/// \param[in] FullyQualify - Convert all template arguments into fully
+/// qualified names.
+NestedNameSpecifier *createNestedNameSpecifier(
+ const ASTContext &Ctx, const TagDecl *TD, bool FullyQualify);
+
+/// \brief Create a NestedNameSpecifier for TypedefDecl and its enclosing
+/// scopes.
+///
+/// \param[in] Ctx - the AST Context to be used.
+/// \param[in] TD - the TypedefDecl for which a NestedNameSpecifier is
+/// requested.
+/// \param[in] FullyQualify - Convert all template arguments (of possible
+/// parent scopes) into fully qualified names.
+NestedNameSpecifier *createNestedNameSpecifier(
+ const ASTContext &Ctx, const TypedefNameDecl *TD,
+ bool FullyQualify);
+
----------------
It would probably be better to have a single function to create a `NestedNameSpecifier` from a `TypeDecl` rather than splitting up these two cases.
================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:133
@@ +132,3 @@
+/// \param[in] Ctx - the AST Context to be used.
+/// \param[in] TD - the TypedefDecl for which a NestedNameSpecifier is
+/// requested.
----------------
TypedefDecl -> TypedefNameDecl
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:31
@@ +30,3 @@
+
+static NestedNameSpecifier *getFullyQualifiedNameSpecifier(
+ const ASTContext &Ctx, NestedNameSpecifier *scope);
----------------
NameSpecifier -> NestedNameSpecifier
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:51
@@ +50,3 @@
+ }
+ } else {
+ NNS = createNestedNameSpecifierForScopeOf(Ctx, ArgTDecl, true);
----------------
`ArgTDecl` could be null here (for instance, if the `TemplateName` is a dependent template name).
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:87
@@ +86,3 @@
+
+static const Type *getFullyQualifiedLocalType(const ASTContext &Ctx,
+ const Type *TypePtr) {
----------------
What do you mean by 'Local' here?
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:89
@@ +88,3 @@
+ const Type *TypePtr) {
+ // We really just want to handle the template parameter if any ....
+ // In case of template specializations iterate over the arguments and
----------------
I don't think this comment is accurate, just delete it?
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:98-99
@@ +97,4 @@
+ I != E; ++I) {
+ // cheap to copy and potentially modified by
+ // getFullyQualifedTemplateArgument
+ TemplateArgument Arg(*I);
----------------
Comment should start with a capital letter and end in a period.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:105
@@ +104,3 @@
+
+ // If desugaring happened allocate new type in the AST.
+ if (MightHaveChanged) {
----------------
This comment is inaccurate; you're not doing desugaring. (The name of `DesArgs` is also inappropriate; `QualArgs` or `FQArgs` or similar would better express the intent.)
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:107-110
@@ +106,6 @@
+ if (MightHaveChanged) {
+ QualType QT = Ctx.getTemplateSpecializationType(
+ TST->getTemplateName(), DesArgs.data(), DesArgs.size(),
+ TST->getCanonicalTypeInternal());
+ return QT.getTypePtr();
+ }
----------------
Don't you need to form a fully-qualified template name here too?
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:150-153
@@ +149,6 @@
+ if (const NamespaceDecl *NS = dyn_cast<NamespaceDecl>(DC)) {
+ while (NS && NS->isInline()) {
+ // Ignore inline namespace;
+ NS = dyn_cast_or_null<NamespaceDecl>(NS->getDeclContext());
+ }
+ if (NS->getDeclName()) return TypeName::createNestedNameSpecifier(Ctx, NS);
----------------
This won't do the right thing if you reach an `extern "C"` or `extern "C++"` context. You can use `getRedeclContext` to skip those.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:154
@@ +153,3 @@
+ }
+ if (NS->getDeclName()) return TypeName::createNestedNameSpecifier(Ctx, NS);
+ return nullptr; // no starting '::', no anonymous
----------------
This will crash if you reached the `TranslationUnitDecl` or `NS` otherwise became null.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:155
@@ +154,3 @@
+ if (NS->getDeclName()) return TypeName::createNestedNameSpecifier(Ctx, NS);
+ return nullptr; // no starting '::', no anonymous
+ } else if (const TagDecl *TD = dyn_cast<TagDecl>(DC)) {
----------------
This is the wrong result if you reach an anonymous namespace nested within a named namespace.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:166
@@ +165,3 @@
+ const ASTContext &Ctx, NestedNameSpecifier *Scope) {
+ // Return a fully qualified version of this name specifier
+ switch (Scope->getKind()) {
----------------
Make this a function doc comment. And likewise for the similar cases below.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:173-175
@@ +172,5 @@
+ return TypeName::createNestedNameSpecifier(Ctx, Scope->getAsNamespace());
+ case NestedNameSpecifier::NamespaceAlias:
+ return TypeName::createNestedNameSpecifier(
+ Ctx, Scope->getAsNamespaceAlias()->getNamespace()->getCanonicalDecl());
+ case NestedNameSpecifier::Identifier:
----------------
Why do you desugar namespace aliases here?
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:177-178
@@ +176,4 @@
+ case NestedNameSpecifier::Identifier:
+ // A function or some other construct that makes it un-namable
+ // at the end of the TU. FIXME: How to report this?
+ return Scope;
----------------
This indicates that the `NestedNameSpecifier` is dependent. Leaving this component alone seems like the right thing to do, but you should still fully-qualify the prefix.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:186
@@ +185,3 @@
+ const TagDecl *TD = nullptr;
+ if (const TagType *TagDeclType = dyn_cast<TagType>(Type)) {
+ TD = TagDeclType->getDecl();
----------------
Do not use `dyn_cast` on a `Type*`, it won't do what you want with type sugar. Use `Type->getAs<TagType>()` instead.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:209-212
@@ +208,6 @@
+
+ const NamedDecl *Outer =
+ llvm::dyn_cast_or_null<NamedDecl>(Decl->getDeclContext());
+ const NamespaceDecl *OuterNS =
+ llvm::dyn_cast_or_null<NamespaceDecl>(Decl->getDeclContext());
+ if (Outer && !(OuterNS && OuterNS->isAnonymousNamespace())) {
----------------
Call `getRedeclContext` here to skip `extern "C"` and the like.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:213
@@ +212,3 @@
+ llvm::dyn_cast_or_null<NamespaceDecl>(Decl->getDeclContext());
+ if (Outer && !(OuterNS && OuterNS->isAnonymousNamespace())) {
+ if (const CXXRecordDecl *CxxDecl =
----------------
If the outer context is an anonymous namespace, you should skip over it; it might be within another namespace.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:214-234
@@ +213,23 @@
+ if (Outer && !(OuterNS && OuterNS->isAnonymousNamespace())) {
+ if (const CXXRecordDecl *CxxDecl =
+ llvm::dyn_cast<CXXRecordDecl>(Decl->getDeclContext())) {
+ if (ClassTemplateDecl *ClassTempl =
+ CxxDecl->getDescribedClassTemplate()) {
+ // We are in the case of a type(def) that was declared in a
+ // class template but is *not* type dependent. In clang, it
+ // gets attached to the class template declaration rather than
+ // any specific class template instantiation. This result in
+ // 'odd' fully qualified typename:
+ //
+ // vector<_Tp,_Alloc>::size_type
+ //
+ // Make the situation is 'useable' but looking a bit odd by
+ // picking a random instance as the declaring context.
+ if (ClassTempl->spec_begin() != ClassTempl->spec_end()) {
+ Decl = *(ClassTempl->spec_begin());
+ Outer = llvm::dyn_cast<NamedDecl>(Decl);
+ OuterNS = llvm::dyn_cast<NamespaceDecl>(Decl);
+ }
+ }
+ }
+
----------------
I don't think this is the right way to handle this situation (it would be better to try to build the nested name specifier for the type, and fail if you can't build one that would name the right context). However, I don't see why you need to handle this at all -- it doesn't make sense to call this function with an instantiation-dependent `QualType`, as the notional "at the end of the translation unit" context has no template parameters in scope. You should probably detect and bail out on instantiation-dependent types earlier in the process.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:240-245
@@ +239,8 @@
+ return TypeName::createNestedNameSpecifier(Ctx, TD, FullyQualified);
+ } else {
+ // Decl's context wasn't a namespace or a TagDecl, which means
+ // it is a type local to a scope, and not accessible at the end
+ // of the TU. FIXME: How to report this?
+ return nullptr;
+ }
+ }
----------------
The context might also be the translation unit here.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:276
@@ +275,3 @@
+ // Ignore inline namespace;
+ Namespace = dyn_cast_or_null<NamespaceDecl>(Namespace->getDeclContext());
+ }
----------------
This is wrong in the same way as the similar code above.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:300
@@ +299,3 @@
+
+QualType TypeName::getFullyQualifiedType(QualType QT, const ASTContext &Ctx) {
+ // Return the fully qualified type, including for any template
----------------
This seems to miss out a lot of cases.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:306
@@ +305,3 @@
+ // qualify and attach the pointer once again.
+ if (llvm::isa<PointerType>(QT.getTypePtr())) {
+ // Get the qualifiers.
----------------
You don't need `llvm::` on `isa`.
================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:404
@@ +403,3 @@
+ QualType FQQT = getFullyQualifiedType(QT, Ctx);
+ return FQQT.getAsString(Policy);
+}
----------------
This call ignores a lot of the things you messed with above. It seems like only a couple of the modifications you make to the type actually affect how it's printed at all.
http://reviews.llvm.org/D15861
More information about the cfe-commits
mailing list