[PATCH] D105457: [clang] Refactor AST printing tests to share more infrastructure

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Looks generally good - few things might be able to be tidied up/simplified, I think?



================
Comment at: clang/unittests/AST/ASTPrint.h:61-62
+private:
+  // Can be specialized for specific node types.
+  bool shouldIgnoreNode(const NodeType *N) { return false; }
 };
----------------
Why is this implemented via specialization but other configuration is implemented via functors (like `Printer` and `PolicyAdjuster`) are done via passed in functors? Might be worth using the same mechanism for all of them - this one can have a default argument value in the ctor so it doesn't have to be specified by all uses of this class/they can get this default behavior as implemented here.


================
Comment at: clang/unittests/AST/DeclPrinterTest.cpp:59
+  return PrintedNodeMatches<Decl>(Code, Args, NodeMatch, ExpectedPrinted,
+                                  FileName, PrinterType<Decl>{PrintDecl},
+                                  PolicyModifier, AllowError);
----------------
Is the `PrinterType<Decl>{}` needed here, or could `PrintDecl` be passed directly/without explicitly constructing the wrapper? (functions should be implicitly convertible to the lambda type, I think?) 

Similarly for all the `PrintingPolicyAdjuster(...)` - might be able to skip that wrapping expression & rely on an implicit conversion.


================
Comment at: clang/unittests/AST/StmtPrinterTest.cpp:55-56
+                   PrintingPolicyAdjuster PolicyAdjuster = nullptr) {
+  return PrintedNodeMatches(Code, Args, NodeMatch, ExpectedPrinted, "",
+                            PrinterType<Stmt>{PrintStmt}, PolicyAdjuster);
+}
----------------
I guess maybe this could be changed (better or worse, not sure) to this:
```
return PrintedNodeMatches<Stmt>(..., PrintStmt, ...);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105457