riccibruno added a comment. Thanks for finishing this.
I don't see why you are adding `dumpPretty`; the point of the `dump` function I added is to dump the `APValue` as the tree-like structure it is. But your `dumpPretty` doesn't do that at all. Instead it is just an alias for `printPretty`. So `dump` does one thing and `dumpPretty` does another completely different thing. ================ Comment at: clang/include/clang/AST/PrettyPrinter.h:222 + /// Whether null pointers should be printed as nullptr or as NULL. + unsigned UseNullptr : 1; + ---------------- This seems to be unrelated. And anyway shouldn't this be inferred from the language mode? ================ Comment at: clang/lib/AST/APValue.cpp:429 return; case APValue::LValue: { bool IsReference = Ty->isReferenceType(); ---------------- What I would do here is continue to take a `const ASTContext &` here, but in the `LValue` case in `TextNodeDumper` just print `<ASTContext required>` if the context is null (which should only happens when debugging since the parameter-less version of `dump()` is not used in the code-base). Trying to do without the context here can result in confusing output. I think it is better to just give-up and tell the user to please pass the `ASTContext` (which again, should only happens in a debugger). Additionally since the context will always be present outside of debugging, the code-path without the context will not be tested. ================ Comment at: clang/lib/AST/APValue.cpp:629 + QualType Ty) const { + ::InternalPrinter(Out, *this, Ty, &Ctx, Ctx.getPrintingPolicy()); +} ---------------- What's up with this inconsistency? One time `OS` and the other time `Out`... ================ Comment at: clang/test/AST/ast-dump-APValue-MemPtr.cpp:4 +// RUN: -ast-dump %s -ast-dump-filter Test \ +// RUN: | FileCheck --strict-whitespace %s + ---------------- Can you add a TODO to remember to add the serialization part when the serialization of `APValue` is fixed? Also can you match full lines? Here the test would still pass if you had written `// CHECK-NEXT: value: MemberPointer &S`. You can use `utils/make-ast-dump-check.sh` + some manual editing to generate the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85144/new/ https://reviews.llvm.org/D85144 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits