[PATCH] D83183: [clang] Rework how and when APValues are dumped

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 2 inline comments as done.
riccibruno added a comment.

Thanks for your comments!

In D83183#2132975 <https://reviews.llvm.org/D83183#2132975>, @aaron.ballman wrote:

> Do none of the JSON tests break from this change?


No, but only because I am not modifying the JSON output at all (the JSON output is still from `APValue::printPretty`).



================
Comment at: clang/include/clang/AST/TextNodeDumper.h:163
 
+  raw_ostream &getOS() { return OS; }
+
----------------
aaron.ballman wrote:
> This is a pretty strange public method; any way to limit its visibility?
I just need it for `dumpAPValueChildren`, but I can make `dumpAPValueChildren` a private method of `TextNodeDumper` instead.


================
Comment at: clang/test/Import/switch-stmt/test.cpp:8
 // CHECK-NEXT: ConstantExpr
+// CHECK-NEXT: value: Int 1
 // CHECK-NEXT: IntegerLiteral
----------------
aaron.ballman wrote:
> I sort of wonder whether we want both the text and the JSON dumpers to dump these as: `value(type): <val>`, as that seems like it produces results that are a bit more well-structured. WDYT?
I'm not sure I follow. The `value` is just a label for the child of the `VarDecl`.
If you look at a more complex example such as:
```
VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s4 'const S4'
|-value: Struct
| |-base: Struct
| | `-fields: Int 0, Union .j Int 0
| |-fields: Int 1, Int 2, Int 3
| |-field: Struct
| `-fields: Int 4, Int 5, Int 6
```

There is no other `value` label.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83183