dblaikie added a comment. In D134453#3826832 <https://reviews.llvm.org/D134453#3826832>, @aaron.ballman wrote:
> In D134453#3825902 <https://reviews.llvm.org/D134453#3825902>, @dblaikie > wrote: > >>>>>> 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. >> >> Hmm - that seems to be to be somewhat of a tangent, though? I think the >> claim that `template<auto> struct t1; struct t2 { }; t1<t2{}> v1;` and then >> rendering `t1<{}>` in a diagnostic is wrong is solid, and we should fix that >> for auto parameters to include the type info that'd be necessary in the >> source code/to disambiguate. (I guess C++20 actually requires the name even >> in non-ambiguous, non-auto cases - so I'd be OK leaning towards the syntax >> requirement, even if it's not necessary to disambiguate (maybe there are >> cases where it is?). >> >> You're suggesting that -ast-print should respect the type as-written by the >> user, even? Not just "enough to be valid C++ code/disambiguate" - and that >> mode might be enough to support @DoDoENT's use case using strings for type >> information? (more addressing this feature request, less about the >> incidental diagnostic quality bug report that's a side issue in this >> discussion) > > I'm mostly looking at it from the perspective of "is it reasonable to add a > new printing policy here and if so, what do we need from it" first and then > "when is it reasonable to enable that printing policy". I think the presence > of `-ast-print` answers the first question that we probably do need *some* > sort of printing policy here because we believe we have at least two > scenarios to support: print for a diagnostic and print for the pretty printer. Ah, OK - I'm not sure that use case motivates a different printing mode, though - skipping to the bottom to continue this thought. > We might even find we want a verbosity level instead of a boolean switch: > print minimal type info, print "disambiguating" type info, print all type > info, though it sort of sounds to me like we only need two modes > (disambiguating mode for diagnostics and all type info for pretty > printing/@DoDoENT). > > (I think the original goal of `-ast-print` was to respect the code as-written > by the user, but I am not convinced we need that strict of a goal. I think > "produces code which compiles and has the same semantics as the original > source" is a more reasonable goal and once we can do that much we can think > about improving fidelity more if we need to.) OK, so it sounds like the printing behavior change (not necessarily with a policy flag) necessary for diagnostics (make it non-ambiguous/not invalid code which is what is currently emitted) would be a good bug fix both for diagnostics and for ast-print? I'm not sure we need anything other than that. So that what is currently printing as `t1<{}>` (ambiguous if the NTTP is of type `auto`, non-ambiguous, but invalid C++ if the NTTP has the specific type already) prints out as `t1<t2{}>` (correct C++ and disambiguates in the case of `auto` ambiguity). Or would you want the non-auto case to print in diagnostics as `t1<{}>` since there's no ambiguity, despite that not being valid C++? And then have a separate policy for ast-print that still prints it as `t1<t2{}>` so it's valid code? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134453/new/ https://reviews.llvm.org/D134453 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
