On Fri, Jan 8, 2016 at 1:54 PM, Philippe Canal <pca...@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@lists.llvm.org><cfe-commits@lists.llvm.org>
> <cfe-commits@lists.llvm.org> Reply-To:
> reviews+d15861+public+e412d660379b2...@reviews.llvm.org, Sterling
> Augustine <saugust...@google.com> <saugust...@google.com> To:
> saugust...@google.com, rich...@metafoo.co.uk CC:
> cfe-commits@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-comm...@lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
