<div dir="ltr">+Mr. Smith for visibility.<br><br>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<br><br><a class="gmail_plusreply" id="plusReplyChip-1" href="mailto:sammccall@google.com" tabindex="-1">+Sam McCall</a>  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.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 11, 2020 at 11:28 PM Jean-Baptiste Lespiau via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div><b>Context and history:</b></div><div><br></div><div>I have found a bug in <a href="https://github.com/google/clif" target="_blank">CLIF</a>, which does not correctly fully qualify templated names when they are nested, e.g.</div><div><br></div><div>::tensorfn::Nested< ::std::variant<Tensor, DeviceTensor> ><br></div><div><br></div><div>should have been:</div><div><br></div><div>::tensorfn::Nested< ::std::variant<::tensorflow::Tensor,::tensorfn::DeviceTensor> ><br></div><div><br></div><div>I tracked it down to </div><div><a href="https://github.com/google/clif/blob/master/clif/backend/matcher.cc#L172" target="_blank">https://github.com/google/clif/blob/master/clif/backend/matcher.cc#L172</a><br></div><div>which calls</div><div><a href="https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp" target="_blank">https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp</a><br></div><div>and the error is exactly at the line, but I could not really understand why.</div><div><a href="https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp#L457" target="_blank">https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp#L457</a><br></div><div><br></div><div>Historically, it has been added by the developers of CLIF (including Sterling Augustine)</div><div><a href="https://github.com/llvm/llvm-project/commit/0dd191a5c4bf27cc8a2b6033436b00f0cbdc4ce7" target="_blank">https://github.com/llvm/llvm-project/commit/0dd191a5c4bf27cc8a2b6033436b00f0cbdc4ce7</a>.<br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><b>Suggestion</b></div><div><b><br></b></div><div> 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).</div><div><br></div><div>In practice, it is still a different display at what QualTypeNames.cpp was doing, as, for example, it displays</div><div><br></div><div>::tensorfn::Nested<::std<b>::__u</b>::variant<tensorflow::Tensor, ::tensorfn::DeviceTensor>><br></div><div><br></div><div>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?^^).</div><div><br></div><div>I am contacting you, so we can discuss:</div><div><br></div><div>- Whether <a href="https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp" target="_blank">QualTypeNames.cpp</a> 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.</div><div>- 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?).</div><div><br></div><div>Thanks!</div><div><br></div><div></div></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>