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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/APValue.cpp:628-629
 
+static bool TryPrintAsStringLiteral(raw_ostream &Out, const ArrayType *ATy,
+                                    const APValue *Data, size_t Size) {
+  if (Size == 0)
----------------
Is there anything you can factor out of `StringLiteral::outputString` and reuse here? At least the single-character printing code seems like something we should try to not overly duplicate.


================
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;
----------------
We should drop all trailing zeroes here, because array initialization from a string literal will reconstruct them.


================
Comment at: clang/lib/AST/APValue.cpp:641-643
+  constexpr size_t MaxN = 36;
+  char Buf[MaxN * 2 + 3] = {'"'}; // "At most 36 escaped chars" + \0
+  auto *pBuf = Buf + 1;
----------------
Prefer `llvm::SmallString<N>` and `push_back`.


================
Comment at: clang/lib/AST/APValue.cpp:645
+
+  // Better than printing a two-digit sequence of 10 integers.
+  StringRef Ellipsis;
----------------
...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.


================
Comment at: clang/lib/AST/APValue.cpp:902
         if (I == 10) {
           // Avoid printing out the entire contents of large arrays.
+          Out << "...}";
----------------
(The existing code is also doing elision here that will be inappropriate for some callers. If you add a `PrintingPolicy` to unambiguously print template arguments, it should disable this check too.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115031