dblaikie added a comment. The test code seems a bit overly general/overengineered for the use case, perhaps? (for instance it has unused parameters like the "Args" parameter - that neither caller passes in a value for, it has accessors (GetPRinted/getNumFoundTypes) when all the code fits on a (largeish) screen & so maybe having public members might be fine for that purpose - maybe all those are boilerplate AST matcher stuff, I guess, which is OK? but wouldn't mind it a bit simpler given the simplicity of the test
& perhaps this could be tested without the full power of AST matchers? Though perhaps that is the easiest way to locate the desired AST node. ================ Comment at: clang/unittests/AST/TypePrinterTest.cpp:26-27 + +using PolicyAdjusterType = + Optional<llvm::function_ref<void(PrintingPolicy &Policy)>>; + ---------------- this is probably unnecessary/redundant - an llvm::function_ref itself can already be empty/none/boolean tested, which should be adequate without wrapping it in the extra layer of Optional? But also, since there's only two callers and they both pass a non-empty functor - seems fine not to even test for empty and always apply the functor. Alternatively, the caller could pass in a PrintingPolicy, instead of mutating the one here? Might be simpler? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104619/new/ https://reviews.llvm.org/D104619 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
