aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM aside from a nit. ================ Comment at: lib/AST/ASTDumper.cpp:1462 const OMPExecutableDirective *Node) { - for (auto *C : Node->clauses()) { - dumpChild([=] { - if (!C) { - ColorScope Color(OS, ShowColors, NullColor); - OS << "<<<NULL>>> OMPClause"; - return; - } - { - ColorScope Color(OS, ShowColors, AttrColor); - StringRef ClauseName(getOpenMPClauseName(C->getClauseKind())); - OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper() - << ClauseName.drop_front() << "Clause"; - } - NodeDumper.dumpPointer(C); - NodeDumper.dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc())); - if (C->isImplicit()) - OS << " <implicit>"; - for (auto *S : C->children()) - dumpStmt(S); - }); - } + for (auto *C : Node->clauses()) + Visit(C); ---------------- `const auto *` ================ Comment at: lib/AST/TextNodeDumper.cpp:276-280 + if (!C) { + ColorScope Color(OS, ShowColors, NullColor); + OS << "<<<NULL>>> OMPClause"; + return; + } ---------------- This pattern is coming up quite frequently. I wonder if we should factor this out into something like: ``` template <typename Ty> bool dumpNullObject(const Ty *Val, StringRef Label) const { if (!Val) { ColorScope Color(OS, ShowColors, NullColor); OS << "<<<NULL>>> " << Label; } return !Val; } ``` So that uses become: ``` if (dumpNullObject(Whatever, "Whatever")) return; ``` Doesn't have to be done as part of this patch, but it seems like it might reduce some redundancy. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56708/new/ https://reviews.llvm.org/D56708 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits