[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()) {
----------------
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&lt;*,std::ratio&lt;1,1000000000&gt; &gt;">
      <DisplayString>{_MyRep} nanoseconds</DisplayString>
      <Expand/>
  </Type>

  <Type Name="std::chrono::duration&lt;*,std::ratio&lt;1,1000000&gt; &gt;">
      <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...


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

https://reviews.llvm.org/D77598

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits