[PATCH] D56708: NFC: Implement OMPClause dump in terms of visitors

[Aaron Ballman via Phabricator via cfe-commits]

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