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
