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