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
