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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
