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