[PATCH] D85144: [clang] Improve Dumping of APValues

[Bruno Ricci via Phabricator via cfe-commits]

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