[PATCH] D135518: [clangAST] support TypeLoc in TextNodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this! Should we make similar changes to the `JSONNodeDumper` as well? (It doesn't seem to have `TypeLoc` support.)



================
Comment at: clang/include/clang/AST/ASTDumperUtils.h:55-56
 static const TerminalColor TypeColor = {llvm::raw_ostream::GREEN, false};
+// Type location names (int, float, etc, plus user defined types)
+static const TerminalColor TypeLocColor = {llvm::raw_ostream::GREEN, false};
 
----------------
I don't know that we need a color specific to type locs; those should behave the same as a type node (after all, it's a type coupled with a source location, basically).


================
Comment at: clang/include/clang/AST/TextNodeDumper.h:337
+  void VisitConstantArrayTypeLoc(ConstantArrayTypeLoc TL);
+  void VisitVariableArrayTypeLoc(VariableArrayTypeLoc TL);
+  void VisitDependentSizedArrayTypeLoc(DependentSizedArrayTypeLoc TL);
----------------
Should you also handle `IncompleteArrayTypeLoc` as well?


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1643
+void TextNodeDumper::VisitRValueReferenceTypeLoc(ReferenceTypeLoc TL) {
+  VisitReferenceType(dyn_cast<ReferenceType>(TL.getType().getTypePtr()));
+}
----------------
These uses of `dyn_cast` can all be switched to `cast` instead because we already know the type we're going to find.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1652
+  VisitConstantArrayType(
+      dyn_cast<ConstantArrayType>(TL.getType().getTypePtr()));
+}
----------------
This one is actually not quite right. You need to go through the `ASTContext` to get to an array type. This should use `ASTContext::getAsConstantArrayType()` instead.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1657
+  VisitVariableArrayType(
+      dyn_cast<VariableArrayType>(TL.getType().getTypePtr()));
+}
----------------
Similarly, `ASTContext::getAsVariableArrayType()`


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1663
+  VisitDependentSizedArrayType(
+      dyn_cast<DependentSizedArrayType>(TL.getType().getTypePtr()));
+}
----------------
`ASTContext::getAsDependentSizedArrayType()`


================
Comment at: clang/lib/AST/TypeLoc.cpp:70-72
+const char *TypeLoc::getTypeLocClassName() const {
+  return TypeNamer().Visit(*this);
+}
----------------
It might make sense for this to return a `StringRef` instead so it's clear that the caller does not own the memory or have to worry about releasing it. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135518