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

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked an inline comment as done.
steveire added inline comments.


================
Comment at: lib/AST/TextNodeDumper.cpp:276-280
+  if (!C) {
+    ColorScope Color(OS, ShowColors, NullColor);
+    OS << "<<<NULL>>> OMPClause";
+    return;
+  }
----------------
aaron.ballman wrote:
> 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.
Something to come back to later I think. It would be more consistent now to have the label on the left side (dumping the various parts of an if() which may be nullptr), and in this OMPClause case, to not have a label on the right side. 

Also - at least the 'OS << "<<<NULL>>>"' in the comment visitor is dead unreachable code.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D56708