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

[Sterling Augustine via cfe-commits]

saugustine added a comment.

Thanks for the reviews. Please take another look when you get a chance.


================
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:
> 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.
My use case is to take a function signature, and communicate to a developer one way to declare the variables they need to call the function.

It does expand entire qualtypes, not just template parameters. (I've updated that description.)

Given the use case, "at the end of the translation unit" is the closest description of where these names would be valid, with the exception that this code avoids relying on any "using" declaration. "using foo::bar; void bat(bar b);", this code would describe foo's parameter as type foo::bar, rather than plain "bar", even though plain "bar" would work at the end of the translation unit.

I have updated the file header's comment to reflect all this, and added a couple of test cases to prove to myself that it does what I have documented. Along the way I have found a couple of places to explicitly mark where one would do things differently if one wanted to change this behavior.

The "ideal interface" idea is a good--and very cool--one, but my use case doesn't call for it.

================
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);
+
----------------
rsmith wrote:
> 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?
I believe the Cling project (from which this code is adapted) uses all those functions internally. So I expect at least one client for those functions.

Let me know if you think hiding this interface is still the right thing.

================
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();
----------------
rsmith wrote:
> 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.
I've rewritten this code a bit, and this problem is solved near the end of this function. (This section itself is now mostly gone.)


http://reviews.llvm.org/D15861