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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits