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; + ---------------- lichray wrote: > rsmith wrote: > > 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. > Added `PrintingPolicy` parameter to `TemplateParamObjectDecl::printAsInit` > and `printAsExpr`, fixed the implementation, and added tests for the effect > of the new `EntireContentsOfLargeArray` bit. > > Whether how diagnosis, codegen, and higher-level printers should make use of > them, I think they're outside the scope of this patch. Generally I agree, we don't need to fix all the places that should be setting this new flag as part of the same patch, but I think at least `-ast-print` should be fixed here, given how obviously broken it currently is. That would also remove the need to add a unit test for the new flag. ================ Comment at: clang/test/SemaTemplate/temp_arg_string_printing.cpp:1 +// RUN: %clang_cc1 -std=c++20 -fsyntax-only -ast-print %s | FileCheck %s + ---------------- Please use something like diagnostics to test that elision happens, rather than `-ast-print`, because `-ast-print` aims to produce valid C++ code that has the same meaning as the input -- this is a canonical example of a case that should *not* elide large array contents. 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
