Bug in QualTypeNames.cpp and adding an option to prepend "::" to fully qualified names.

[David Blaikie via cfe-commits]

+Mr. Smith for visibility.

I'm /guessing/ the right path might be to change the implementation of
getFullyQualifiedName to use the type printing/pretty printer approach with
the extra feature you're suggesting. That way all users get the desired
behavior

+Sam McCall <sammccall at google.com>  who (if I understand correctly) has a
lot to do with the Clang Tooling work - looks like Google's got a bunch of
uses of this function (getFullyQualifiedName) internally in clang tools (I
wonder why that's the case when there are still zero external callers - is
that OK? Or are external users doing something different (better? worse?)
to answer this question - or the tooling uses in LLVM proper just don't
have the same needs?). So probably best not to leave a buggy implementation
lying around - either deleting it, or fixing it.

On Mon, May 11, 2020 at 11:28 PM Jean-Baptiste Lespiau via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Hi,
>
> *Context and history:*
>
> I have found a bug in CLIF <https://github.com/google/clif>, which does
> not correctly fully qualify templated names when they are nested, e.g.
>
> ::tensorfn::Nested< ::std::variant<Tensor, DeviceTensor> >
>
> should have been:
>
> ::tensorfn::Nested<
> ::std::variant<::tensorflow::Tensor,::tensorfn::DeviceTensor> >
>
> I tracked it down to
> https://github.com/google/clif/blob/master/clif/backend/matcher.cc#L172
> which calls
> https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp
> and the error is exactly at the line, but I could not really understand
> why.
>
> https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp#L457
>
> Historically, it has been added by the developers of CLIF
> (including Sterling Augustine)
>
> https://github.com/llvm/llvm-project/commit/0dd191a5c4bf27cc8a2b6033436b00f0cbdc4ce7
> .
> They explained to me, that they pushed this to Clang hoping it would be
> used by other tools and maintained by the community, but that it kind of
> failed at that, and it (i.e. QualTypeName.pp) is not much used, and not
> much maintained.
>
> I was not able to understand this file to provide a fix. On the other
> side, it was easy to understand TypePrinter.cpp and PrettyPrinter.cpp, so I
> tried extending it to fit my need.
>
> *Suggestion*
>
>  As I wanted fully qualified types (it's usually more convenient for tools
> generating code), to prevent some complex errors), I added ~10 lines that
> add an option to prepend "::" to qualified types (see the patch).
>
> In practice, it is still a different display at what QualTypeNames.cpp was
> doing, as, for example, it displays
>
> ::tensorfn::Nested<::std*::__u*::variant<tensorflow::Tensor,
> ::tensorfn::DeviceTensor>>
>
> but I was able to solve my issues. More generally, it is more verbose, as
> it displays the exact underlying type, including default parameter types in
> template arguments. So it's verbose, but it's correct (what is best?^^).
>
> I am contacting you, so we can discuss:
>
> - Whether QualTypeNames.cpp
> <https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp> is
> something useful for the Clang project, whether you think we should fix the
> bug I have found (but I cannot, as I do not understand it), or whether we
> should deprecate it, or modify the current printing mechanism
> (TypePrinter.cpp and PrettyPrinter.cpp) to add more options to tune the
> display in ways people may want to.
> - Whether adding the CL I have attached is positive, and if yes, what
> should be done in addition to that (does it need tests? Are there more
> types that we may want to prepend "::" to its fully qualified name?).
>
> Thanks!
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://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/20200512/a1fa7a4a/attachment.html>