[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/include/clang/AST/PrettyPrinter.h:288
+  /// template parameters, no matter how many elements there are.
+  unsigned EntireContentsOfLargeArray : 1;
+
----------------
It looks like nothing is setting this to `true` yet, so that part of this patch seems untested. The places I think we want to set this to `true` are:

1) When forming types in a diagnostic, if we have two types that differ only in the contents of large arrays. That's checked in here: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTDiagnostic.cpp#L259

    I think what we'd want is: if we still have a collision between type names after comparing the canonical strings (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTDiagnostic.cpp#L291), then set this policy to `true` for the rest of the function, and recompute `S` and `CanS`.

    This should be detectable in a case such as:

```
void f(X<{"some very long string that differs here: x"}>);
void g() {
  f(X<{"some very long string that differs here: y"}>());
}
```

... where the diagnostic should include the whole string, not elide the differing portion at the end.

2) In the `-ast-print` output: when pretty-printing as code, we want to produce correct, non-elided template arguments. I think this will require passing a printing policy into `TemplateParamObjectDecl::printAsInit` and `::printAsExpr` (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclTemplate.cpp#L1509). This should be detectable in the output of `-ast-print`, where we do not want to elide any part of template arguments. Perhaps `StmtPrinter` and `DeclPrinter` should enable this `PrintingPolicy` flag?

3) When generating debug information: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L230

    It's generally important that debug information has complete type names.


================
Comment at: clang/include/clang/Basic/CharInfo.h:50
 LLVM_READNONE inline bool isASCII(uint32_t c) { return c <= 127; }
+LLVM_READNONE inline bool isASCII(int64_t c) { return c <= 127; }
 
----------------
I think this should also return false for negative values.


================
Comment at: clang/lib/AST/APValue.cpp:630-631
+                                    const PrintingPolicy &Policy,
+                                    const ArrayType *ATy, const APValue *Data,
+                                    size_t Size) {
+  if (Size == 0)
----------------
Consider passing in an `ArrayRef` here instead of forming one below.


================
Comment at: clang/lib/AST/APValue.cpp:654
+
+  for (auto &Val : ArrayRef<const APValue>(Data, Size)) {
+    auto Char64 = Val.getInt().getExtValue();
----------------
I think `ArrayRef<T>` and `ArrayRef<const T>` are effectively the same thing (you'd need `MutableArrayRef` to remove the implied `const`), so you should be able to just use `ArrayRef<APValue>` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115031