[PATCH] D77598: Integral template argument suffix and cast printing

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/AST/TemplateBase.cpp:71-72
 
   if (T->isBooleanType() && !Policy.MSVCFormatting) {
     Out << (Val.getBoolValue() ? "true" : "false");
   } else if (T->isCharType()) {
----------------
reikdas wrote:
> rnk wrote:
> > rsmith wrote:
> > > rsmith wrote:
> > > > rnk wrote:
> > > > > rsmith wrote:
> > > > > > reikdas wrote:
> > > > > > > rsmith wrote:
> > > > > > > > rsmith wrote:
> > > > > > > > > It looks like `MSVCFormatting` wants `bool` values to be printed as `0` and `1`, and this patch presumably changes that (along with the printing of other builtin types). I wonder if this is a problem in practice (eg, if such things are used as keys for debug info or similar)...
> > > > > > > > Do we need to suppress printing the suffixes below in `MSVCFormatting` mode too?
> > > > > > > @rsmith The tests pass, so that is reassuring at least. Is there any other way to find out whether we need to suppress printing the suffixes for other types in MSVCFormatting mode?
> > > > > > @rnk Can you take a look at this? Per rG60103383f097b6580ecb4519eeb87defdb7c05c9 and PR21528 it seems like there might be specific requirements for how template arguments are formatted for CodeView support; we presumably need to make sure we still satisfy those requirements.
> > > > > Probably. This change looks like it preserves behavior from before when `MSVCFormatting` is set, so I think this is OK. I checked, my version of MSVC still uses 1/0 in debug info for boolean template args.
> > > > My concern is that we're changing the behavior for the other cases below in `MSVCFormatting` mode, not that we're changing the behavior for `bool`. If we have specific formatting requirements in `MSVCFormatting` mode, they presumably apply to all types, not only to `bool`, so we should be careful to not change the output in `MSVCFormatting` mode for any type.
> > > @rnk Ping.
> > I think we do need to suppress the suffixes for MSVCFormatting. Consider this visualizer I found in the stl.natvis file:
> > ```
> >   <Type Name="std::chrono::duration<*,std::ratio<1,1000000000> >">
> >       <DisplayString>{_MyRep} nanoseconds</DisplayString>
> >       <Expand/>
> >   </Type>
> > 
> >   <Type Name="std::chrono::duration<*,std::ratio<1,1000000> >">
> >       <DisplayString>{_MyRep} microseconds</DisplayString>
> >       <Expand/>
> >   </Type>
> > ```
> > 
> > Adding L or ULL after 1000000 has the potential to break the visualizer.
> > 
> > However, in general, I don't think we need to freeze this code in amber. It's not like we put a lot of thought into making this code produce MSVC-idential names when we started using it in CodeView debug info. We just used it and debug issues as they come up.
> > 
> > I think a good rule of thumb for making changes is probably to ask if the main STL data structure names include this template argument feature. So, auto-typed non-type template parameters probably aren't an issue.
> > 
> > ---
> > 
> > Separately, I wish the stl.natvis file was part of the github.com/microsoft/STL project so I could just link to it...
> Just to clarify - is an MSVCFormatting specific change required in this patch? (do we need to suppress suffixes and casts for MSVCFormatting printing policy?)
> And is there anyone we need to notify about this patch before it lands (to make sure nothing breaks)?
Yes, as explained above, this change is likely to break visualizers in Visual Studio. Please preserve the existing behavior of integer printing in debug info when targeting a Windows MSVC environment, and add a test for it.


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

https://reviews.llvm.org/D77598