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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
