[PATCH] D115031: [AST] Print NTTP args as string-literals when possible

[Zhihao Yuan via Phabricator via cfe-commits]

lichray marked 5 inline comments as done.
lichray added inline comments.


================
Comment at: clang/lib/AST/APValue.cpp:637-639
+  // Nothing we can do about a sequence that is not null-terminated
+  if (!Data[--Size].getInt().isZero())
+    return false;
----------------
lichray wrote:
> lichray wrote:
> > rsmith wrote:
> > > lichray wrote:
> > > > rsmith wrote:
> > > > > We should drop all trailing zeroes here, because array initialization from a string literal will reconstruct them.
> > > > Are you sure? `MyType<{""}>` and `MyType<{"\0\0\0"}>` are different types.
> > > That's a separate bug. `APValue`'s printing is not intended to identify the type of the result, only to identify the value in the case where we already know the type. Eg, we don't print a type suffix on a pretty-printed integer value. For the specific case of template parameters, it's the responsibility of the template argument printing code to uniquely identify the template argument.
> > > 
> > > When a template parameter has a deduced class type, we should include that type in the printed output in order to uniquely identify the specialization, but we don't, because that's not been implemented yet. When that's fixed, we should print those cases as `MyType<Str<char, 1>{""}>` and `MyType<Str<char, 4>{""}>`. This is necessary anyway, because we can't in general assume that the resulting value is enough to indicate what type CTAD will have selected.
> > > 
> > > We already handle this properly in some cases, but see this FIXMEs: https://github.com/llvm/llvm-project/blob/6c2be3015e07f25912b8cd5b75214c532f568638/clang/lib/AST/TemplateBase.cpp#L433
> > > 
> > > See also some related issues: https://godbolt.org/z/YhcdMYx6n (note that the non-struct enum case is handled properly but the enum-in-struct case is not).
> > > That's a separate bug. [...]
> > > 
> > > When a template parameter has a deduced class type, we should include that type in the printed output in order to uniquely identify the specialization, but we don't, because that's not been implemented yet. When that's fixed, we should print those cases as `MyType<Str<char, 1>{""}>` and `MyType<Str<char, 4>{""}>`. This is necessary anyway, because we can't in general assume that the resulting value is enough to indicate what type CTAD will have selected.
> > >
> > 
> > But it looks like a feature rather than a bug? String literals provided both type and value to emphasis a structural type's value for equivalence comparison. The reason of why `MyType<{""}>` and `MyType<"\0\0\0">` are different looks more obvious to me.
> > 
> > > See also some related issues: https://godbolt.org/z/YhcdMYx6n (note that the non-struct enum case is handled properly but the enum-in-struct case is not).
> > 
> > I'm fine if a specific NTTP, `A`, is treated differently from `auto`. But the last case is similar to this https://godbolt.org/z/YcscTrchM Which I thought about printing something like `Y<{(E[]){97, 98}}>` :)
> That discussion looks OT... For now, not shrinking `"\0\0\0"` down to `""` follows the existing logic, i.e., we are printing `{0, 0, 0}` even though it's an array.
> 
Nvm, I did not realize that null characters cannot be safely escaped with just `\0` without looking at the characters that follow it. Dropping that case and fallback to print the integer sequence.


================
Comment at: clang/lib/AST/APValue.cpp:645
+
+  // Better than printing a two-digit sequence of 10 integers.
+  StringRef Ellipsis;
----------------
rsmith wrote:
> ...except that some callers want output that uniquely identifies the template argument, and moreover some callers want output that is valid C++ code that produces the same template-id.
> 
> It'd be better to do this behind a `PrintingPolicy` flag that the diagnostics engine sets. But we'd want to turn that flag off during template type diffing.
Added `Policy.EntireContentsOfLargeArray`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115031