[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

Thu Sep 29 11:50:59 PDT 2022

aaron.ballman added a comment.

In D134453#3824423 <https://reviews.llvm.org/D134453#3824423>, @dblaikie wrote:

> In D134453#3823141 <https://reviews.llvm.org/D134453#3823141>, @DoDoENT wrote:
>
>>> Generally the way to do it, if you want to introspect into the type, its template parameters, etc, then yes, keeping pointers to the AST or reparsing the name with Clang as-needed, would be the way to go - or walking the AST up-front and generating your own data structure (not necessarily a string) with the data you want in some structured format.
>>
>> This is precisely what I'm trying to avoid. Source files that include AST-related headers are extremely slow to compile, so I want to build a facade around the code that interacts with clang to keep my build times as low as possible. Thus, I will pay for the slow compilation only in a couple of files.
>>
>>> Sorry, I'm confused - it sounded like you didn't actually want a string, though - you want the type information to make various semantic-aware choices in your tool, yes?
>>
>> At the end of the line, I will need a string representation of the type name, at least in my current approach.
>
> Right - I'm trying to understand what your overall goals are and what clang needs to facilitate them, if it's suitable to do so. And it seems to me that the right way to support them, is for your approach to change - not to rely on the type printing to communicate certain information, but to use the AST to get the information you need (& if you want to encode that in a string, that's up to you - I'd suggest encoding it in some more semantic data structure (a struct, with variables, lists/vectors, etc, describing the properties you want - so you don't have to rely on what clang does/doesn't include in the text and have to parse that information out of the text later on with your own home-grown C++ type parsing code, I guess)).

Whenever possible, interrogating the AST directly should be preferred to using strings to hold the data.

>>> Again, I'm not advocating for the printing as-is, I think adding the top level name that disambiguates would be a good thing - and I think the GCC and MSVC examples somewhat show why adding all the other layers would be harmful to readability - there's a lot of text in those messages that doesn't directly help the user and gets in the way of identifying the important differences between the type names.
>>
>> I think this is a matter of taste. In the example that you've shown, I personally prefer the verbosity of GCC and don't see it as "less readable", but as "more informative". However, I do understand that some people may prefer the output you suggested. What about making this configurable, i.e. behind some clang flag, so that developers that prefer verbosity can enable that?
>
> I don't think that's the right choice for clang - though I'm not the only decider here. Be great to hear from @aaron.ballman at some point.
>
> My perspective on this issue at the moment is that we should fix any case where we print an ambiguous type (so `template<auto A> struct t1;` should never produce `t1<{}>` and should instnead produce `t1<t2{}>`) - but I don't think extra {} should be added where the language doesn't require it and where it isn't necessary to disambiguate.

My thinking is along a somewhat different line of approach. Clang has `-ast-print` which is intended to pretty print the AST back to the original source code without losing information. It is a `-cc1` option and not a driver option (though we do document it a little bit here https://releases.llvm.org/15.0.0/tools/clang/docs/HowToSetupToolingForLLVM.html), so it's not something we expose trivially to users. This option has always been aspirational -- we'd love it to pretty print back to the original source, but we know there are plenty of cases where we don't manage it and we typically don't require people to maintain it except superficially. Now that we have clang-format, I think its utility is largely theoretical in some ways. Despite its known issues, I am aware of at least some users who make use of the functionality (for various reasons) and I am not 100% convinced we'll get community agreement to drop support for it as a failed experiment despite my personal feelings that the functionality is more trouble than it's worth, especially because I'm reasonably certain some other functionality relies on it (ARC rewriting maybe?) and we'd have to figure out what to do with that.

So my thinking is: if we're going to have `-ast-print`, we should maintain it, and part of maintaining it means having the ability for it to print this code back to compilable source with the same semantics as the original source. Our current behavior with the example is... not particularly fantastic: https://godbolt.org/z/88eK3q869. Alternatively, we could remove `-ast-print` as an option and narrow its focus to only the internal uses we need it for.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134453/new/

https://reviews.llvm.org/D134453