Ah, thanks for the context/update! On Tue, Jun 2, 2020 at 12:37 AM Jean-Baptiste Lespiau <jblesp...@google.com> wrote:
> Yes it was: https://reviews.llvm.org/D80800#2065643 > > Thanks to Sam, I understand the situation better, but I have been slowed > down quite a lot by the fact that > (a) the logic to print types is duplicated both in TypePrinter.cpp and > NamedDecl.{h, cpp}. There are 2 different code-paths that are printing the > fully qualified namespaces. Thus, it probably means that to implement this > feature, we need to do it for both, and test it for both. > (a) there are lacking tests, so I have to add tests that should have been > added a long time ago (thus, I have to understand the Matcher API, to be > able to get back a specific qualified, to be able to retrieve it and print > it, to check the output). > > I did spend some time during the weekend (as I am doing this in addition > to my normal work, as this is not necessary for my project). So given the > amount of work required, I am also looking if I cannot fix the bug in CLIF > without such change in Clang, that, I must admit, will be difficult for me > to get. Initially, I thought it would be at most a 10h fix, and now it's > eating me up, so maybe I should stop for my own sake :P > > On Tue, Jun 2, 2020 at 2:47 AM David Blaikie <dblai...@gmail.com> wrote: > >> Did this end up moving to a review thread? Could someone include a >> link to that phab review, if it did. >> >> Otherwise - I'd still be curious about the answer to Sam's question >> about SuppressUnwrittenScope & see if the inline namespaces could be >> avoided & then going for the "modifying getFullyQualifiedName() to do >> something roughly equivalent but less buggy using the pretty-printer" >> >> On Fri, May 29, 2020 at 7:06 AM Sam McCall <sammcc...@google.com> wrote: >> > >> > Sorry about ignoring this, I didn't really have an opinion - clang is >> fairly full of helper functions that don't quite do what you expect. >> > This does look like a bug though. >> > >> > Generally for printing types, PrettyPrinter is the way to go. So I'm >> supportive of any of: >> > - adding the option to explicitly root names at the global namespace >> > - modifying CLIF to use a PrintingPolicy directly >> > - modifying getFullyQualifiedName() to do something roughly equivalent >> but less buggy using the pretty-printer >> > - deprecating getFullyQualifiedName() >> > >> > > The presence of inline namespaces is about the only bit I'd find a >> touch questionable >> > Does SuppressUnwrittenScope do what you want? >> > >> > I did miss the patch attached to this, we do apparently have docs that >> say mailing patch files to the *-commits@ mailing lists is a good way to >> get them reviewed, but IMO those docs are out of date - Phabricator is the >> way to go. Feel free to send it to me. >> > It will need a test, I think >> llvm-project/clang/unittests/AST/DeclPrinterTest.cpp is probably the best >> place to add one. >> > >> > On Sat, May 23, 2020 at 10:22 PM Jean-Baptiste Lespiau < >> jblesp...@google.com> wrote: >> >> >> >> Hi, >> >> >> >> should we submit this CL, adding the option to prepend classes and >> struct fully qualified names with "::"? >> >> >> >> This can then allow people to slowly switch to the new function, and I >> can close my fix/close my bug in CLIF. I can do more on this CL if it is >> needed, but I would like to be sure this will get submitted and I will not >> waste my time, and know exactly what you want me to do. >> >> >> >> Possible options include: >> >> - adding the full context, with the bug, in the CL description >> >> - adding documentation on `getFullyQualifiedName` expliciting in which >> context the bug can occur >> >> - adding a FullyQualifiedTypePrinter() that will take an AST or a >> TypePolicy, and set the correct fields, so one can do >> QualType.getAsString(FullyQualifiedTypePrinter(ast)) to replace the >> hereabove one. >> >> - add some tests, but I do not know where and how. >> >> >> >> When this few lines are submitted, I can update google3 for clang, and >> I can start changing some internal occurrences, and finally fix the CLIF >> bug. >> >> >> >> Thanks! >> >> >> >> On Tue, May 12, 2020 at 11:19 PM Jean-Baptiste Lespiau < >> jblesp...@google.com> wrote: >> >>> >> >>> >> >>> >> >>> On Tue, May 12, 2020 at 11:11 PM David Blaikie <dblai...@gmail.com> >> wrote: >> >>>> >> >>>> On Tue, May 12, 2020 at 12:40 PM Jean-Baptiste Lespiau < >> jblesp...@google.com> wrote: >> >>>>> >> >>>>> Hi, >> >>>>> >> >>>>> thanks for your answer. >> >>>>> >> >>>>> Just a few remarks: >> >>>>> >> >>>>> 1. I imagine that we cannot know if people are using >> getFullyQualifiedName. In particular, people could have developed their own >> internal tools, thus we cannot be aware of all callers. I do not know >> Clang's policy, but can we delete or completely change a function without >> deprecating it first? >> >>>> >> >>>> >> >>>> The LLVM project offers little/no API guarantees - >> potentially/especially for a function with no use in-tree. But, yes, as >> Googlers we'd be encouraged not to commit an API change knowing this might >> cause widespread internal breakage without a plan/pre-emptively fixing >> things. >> >>>> >> >>>>> >> >>>>> I was imagining that the process was to deprecate it, document the >> case where it can be incorrect, and that in a next release it gets >> deleted/replaced (or someone steps up to fix it). >> >>>> >> >>>> >> >>>> Sometimes deprecation is used - certain APIs have a lot of out of >> tree surface area >> >>>> >> >>>>> >> >>>>> 2. As example of different behaviors: >> >>>>> (a) the qual_type.getAsString() will print implementation namespace >> details (e.g. ::std::__u::variant instead of std::variant). >> >>>> >> >>>> >> >>>> Yep, that's probably not ideal for most source generating use cases. >> >>>> >> >>>>> >> >>>>> (b) It will also display default template arguments, e.g. >> >>>>> template <T = void> >> >>>>> class MyClass >> >>>>> is printed as MyClass (I think) in getFullyQualifiedName, while >> getAsString() will use MyClass<void>. >> >>>> >> >>>> >> >>>> That seems better/correct - did CLIF actually want/rely on/benefit >> from the old behavior of only printing the template name? >> >>> >> >>> >> >>> I did check to replace all of the getFullyQualifiedName in CLIF with >> the new version: it worked like a charm. The error messages are just >> changed accordingly (so they will have longer types). >> >>> And the generated code was also less verbose. >> >>>> >> >>>> >> >>>>> >> >>>>> (c) the example of the nested template argument above. >> >>>> >> >>>> >> >>>> Which seems just wrong/inconsistent/not-behaving-to-spec to me - I >> don't imagine any caller actually wanted that behavior? >> >>> >> >>> >> >>> I agree. >> >>>> >> >>>> >> >>>>> >> >>>>> >> >>>>> At the end,what matters is that getAsString() is semantically >> always correct, even if it can be overly verbose. >> >>>> >> >>>> >> >>>> The presence of inline namespaces is about the only bit I'd find a >> touch questionable. (Hopefully Sam can chime in on some of that) >> >>> >> >>> >> >>> Me too, I would be curious if it is easy to remove. >> >>>> >> >>>> >> >>>>> >> >>>>> I tried to fix getFullyQualifiedName, but I do not understand its >> code. >> >>>>> >> >>>>> 3. When the goal we want to reach has been decided, there is also >> the question on how to transition from the current state to the next state, >> and who can help with that. To be way clearer, I won't be able to fix all >> of Google internal usage of this function (I am not at all working on >> Clang, Clif, or any tools, I just hit the bug and decided to look into it, >> and it happened that I found a fix). Thus, if we can do the changes using >> iterative steps, such as >> >>>>> (a) add the new "::" prefix configuration, >> >>>>> (b) Deprecate/document the fact that getFullyQualifiedName has a >> bug, and people should move to use qual_type.getAsString(Policy) instead, >> using "FullyQualifiedName" and "GlobalScopeQualifiedName". We can for >> example provide : >> >>>> >> >>>> >> >>>> It'd still fall to one of us Googlers to clean this all up inside >> Google - since we build with -Werror, it's not like folks'll just get soft >> encouragement to migrate away from the API, either the warning will be on >> and their build will be broken (in which case we'll probably pick it up >> when we integrate changes from upstream & have to fix it to complete that >> integration) or no signal, and it'll break when the function is finally >> removed. >> >>>> >> >>>>> >> >>>>> std::string GetFullyQualifiedCanonicalType(QualType qual_type, >> const ASTContext &ast,) >> >>>>> clang::PrintingPolicy >> print_policy(ast.GetASTContext().getLangOpts()); >> >>>>> print_policy.FullyQualifiedName = 1; >> >>>>> print_policy.SuppressScope = 0; >> >>>>> print_policy.PrintCanonicalTypes = 1; >> >>>>> print_policy.GlobalScopeQualifiedName = 1; >> >>>>> QualType canonical_type = qual_type.getCanonicalType(); >> >>>>> return canonical_type.getAsString(print_policy); >> >>>>> } >> >>>>> (c) Then, people can upgrade asynchronously, maybe someone will be >> unhappy with this behavior and will suggest something else, etc. >> >>>>> >> >>>>> If we just blindly delete it, it means for example that we have to >> fix all usages in Google to be able to upgrade Clang. It may be the >> approach that is decided we should follow, but I just wanted to make it >> clear that I will not be able to do that job^^ While, having an incremental >> fix in Clang, and then fix Clif is doable as it does not imply to fix all >> calls in one go. >> >>>>> >> >>>>> I just wanted to develop these points. >> >>>> >> >>>> >> >>>> Sure enough - appreciate the awareness of the cost to external >> clients, to be sure. >> >>>> >> >>>> - Dave >> >>>> >> >>>>> >> >>>>> >> >>>>> Thanks! >> >>>>> >> >>>>> On Tue, May 12, 2020 at 7:59 PM David Blaikie <dblai...@gmail.com> >> wrote: >> >>>>>> >> >>>>>> +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 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, 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 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
