[PATCH] D104619: [clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

[David Blaikie via Phabricator via cfe-commits]

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