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