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

Wed Jan 13 11:50:52 PST 2016

On Fri, Jan 8, 2016 at 1:54 PM, Philippe Canal <pcanal at fnal.gov> wrote:

> Hi Richard,
>
> In our use (Persistency of C++ objects, including the (meta)description of
> their data content as well as use the name as key to access the class'
> reflection information at run-time), the 'context' is indeed always the "at
> the end of the translation unit" (or global scope).
> Thus (for us),
>
> > 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>`?
>
> It indeed either `B::TX` or `std::tuple<A::X>` depending on whether the
> typedefs are being desugared or not.
>
> > from within namespace `B`
>
> Within our API there is no way to actually express this (and I can't think
> of a semantic where it would make sense). Thus by construction it is always
> the "end of the translation unit" case.
>
> > The "ideal interface" idea is a good--and very cool--one, but my use
> case doesn't call for it.
>
> This also our situation. In addition, I do think that supporting the
> general case would increase the implementation complexity.  One 'simple'
> adaption is to support also the case with the leading :: (i.e.
> ::std::tuple<::A::X> which would always be safe no matter which context you
> use it).
>

The primary use case for the general "how do I name this thing from *here*"
functionality is automated refactoring tools, where inserting a
fully-qualified name is about the worst (correct) way you can name any
given entity, and isn't even always possible (for instance, if you're
naming an entity from a local scope).

My principal concern here is that we clearly document the design decisions
for these interfaces, so that we know what possible results are / are not
correct. Extending this to support other clients (which want to know how to
name an entity from a specified context) is secondary, and can be postponed
until someone wants to put in the work to make it happen.

Cheers,
> Philippe.
>
>
>
>
> -------- Forwarded Message -------- Subject: Re: [PATCH] D15861: Support
> fully-qualified names for all QualTypes Date: Fri, 8 Jan 2016 19:50:16
> +0000 From: Sterling Augustine via cfe-commits
> <cfe-commits at lists.llvm.org><cfe-commits at lists.llvm.org>
> <cfe-commits at lists.llvm.org> Reply-To:
> reviews+D15861+public+e412d660379b225f at reviews.llvm.org, Sterling
> Augustine <saugustine at google.com> <saugustine at google.com> To:
> saugustine at google.com, richard at metafoo.co.uk CC:
> cfe-commits at lists.llvm.org
>
>
> 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
>
>
>
> _______________________________________________
> cfe-commits mailing listcfe-commits at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160113/65df81cc/attachment-0001.html>