[PATCH] D15861: Support fully-qualified names for all QualTypes

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:32
@@ +31,3 @@
+namespace TypeName {
+///\brief Convert the type into one with fully qualified template
+/// arguments.
----------------
Please ensure there's a space between each `/// ` and the content.

================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:32-33
@@ +31,4 @@
+namespace TypeName {
+///\brief Convert the type into one with fully qualified template
+/// arguments.
+///\param[in] QT - the type for which the fully qualified type will be
----------------
rsmith wrote:
> Please ensure there's a space between each `/// ` and the content.
What do you mean by "fully qualified template arguments" here? Let me give you some examples:

    namespace A {
      struct X {};
    }
    using A::X;
    namespace B {
      using std::tuple;
      typedef typle<X> TX;
      TX t;
      struct A { typedef int X; };
    }

What is the fully-qualified name of `B::t`'s type? Is it `B::TX` or `std::tuple<A::X>` or `std::tuple<::A::X>`? Note that if you want to redeclare `t` from within namespace `B`, `std::tuple<A::X>` will name the wrong type.


Why does this only affect template arguments? Its name suggests it should affect the type as a whole (for instance, in the above case it should produce `std::tuple<...>`, not `tuple<...>`).


Generally, I think this interface needs to specify where the produced names can be used as a name for the specified type, otherwise I don't see how it can ever be reliable. For instance:

> "Generates a name for a type that can be used to name the same type if used at the end of the current translation unit." (eg, `B::TX` or `std::tuple<X>`)

or:

> "Generates a maximally-explicit name for a type that can be used in any context where all the relevant components have been declared. In such a context, this name will either name the intended type or produce an ambiguity error." (eg, `::std::tuple<::A::X>`)

You should also specify what happens when it's not possible to generate such a name. For instance, given:

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

... there's no way to name the type of `x` from outside `f` (which makes certain refactoring operations impossible unless you also move the definition of `struct X`).


I think the ideal interface here would allow you to specify a location where you wish to insert the name, and would produce a "best possible" name for that type for that location, avoiding adding explicit qualification / desugaring wherever possible, but the interface should at least take the context-sensitivity of names into account.

================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:37
@@ +36,3 @@
+///\param[in] Ctx - the ASTContext to be used.
+clang::QualType getFullyQualifiedType(clang::QualType QT,
+                                      const clang::ASTContext &Ctx);
----------------
Remove the `clang::`s; you're already in namespace `clang`.

================
Comment at: include/clang/Tooling/Core/QualTypeNames.h:49-79
@@ +48,33 @@
+
+///\brief Create a NestedNameSpecifier for Namesp and its enclosing
+/// scopes.
+///
+///\param[in] Ctx - the AST Context to be used.
+///\param[in] Namesp - the NamespaceDecl for which a NestedNameSpecifier
+/// is requested.
+clang::NestedNameSpecifier *createNestedNameSpecifier(
+    const clang::ASTContext &Ctx, const clang::NamespaceDecl *Namesp);
+
+///\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.
+clang::NestedNameSpecifier *createNestedNameSpecifier(
+    const clang::ASTContext &Ctx, const clang::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.
+clang::NestedNameSpecifier *createNestedNameSpecifier(
+    const clang::ASTContext &Ctx, const clang::TypedefNameDecl *TD,
+    bool FullyQualify);
+
----------------
Is it useful to expose the intermediary functionality to generate fully-qualified `QualType`s and `NestedNameSpecifier`s rather than only exposing functionality to generate `string`s? What are the intended uses of these functions?

================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:341-343
@@ +340,5 @@
+          llvm::dyn_cast<ElaboratedType>(QT.getTypePtr())) {
+    // Intentionally, we do not care about the other compononent of
+    // the elaborated type (the keyword) as part of the name
+    // normalization is to remove it.
+    Prefix = ETypeInput->getQualifier();
----------------
This will not work in C, or if the type name is normally shadowed by a variable or function. You sometimes need to keep the keyword.


http://reviews.llvm.org/D15861