+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 <sammcc...@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@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@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
