[PATCH] D55492: Implement Attr dumping in terms of visitors

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked 3 inline comments as done.
steveire added inline comments.


================
Comment at: include/clang/AST/AttrVisitor.h:21
+
+namespace attrvisitor {
+
----------------
aaron.ballman wrote:
> I don't think the namespace adds value.
I think the point is to separate the implementation detail. I don't care either way, but the other visitors should be changed first and this one can follow the same pattern. 


================
Comment at: include/clang/AST/AttrVisitor.h:23-24
+
+template <typename T> struct make_ptr { using type = T *; };
+template <typename T> struct make_const_ptr { using type = const T *; };
+
----------------
aaron.ballman wrote:
> Oh, this is cute. We have StmtVisitor which does not use a namespace, but defines these functions. Then we duplicate these function definitions in each of the comment, decl, and attribute visitors under distinct namespaces so we don't get ODR violations.
> 
> I think we should add `llvm::make_const_ptr` to STLExtras.h so we can use the same definition from everywhere. We can use `std::add_pointer` in place of `make_ptr`, because that is a one-to-one mapping, so we can drop `make_ptr` entirely. However, I don't think we can use `std::add_pointer<std::add_const>` similarly because there's no way to bind that to the template template parameter directly. I'm happy to make these changes if you'd like.
Yes, I've thought about this duplication too. If you deduplicate, I'll rebase this change.  


================
Comment at: include/clang/AST/AttrVisitor.h:27
+/// A simple visitor class that helps create attribute visitors.
+template <template <typename> class Ptr, typename ImplClass,
+          typename RetTy = void, class... ParamTys>
----------------
aaron.ballman wrote:
> s/class/typename
I think c++17 is the first to allow typename here. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D55492