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 <https://llvm.org/docs/Contributing.html#how-to-submit-a-patch> 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 <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
