aaron.ballman added inline comments.
================
Comment at: include/clang/AST/AttrVisitor.h:21
+
+namespace attrvisitor {
+
----------------
I don't think the namespace adds value.
================
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 *; };
+
----------------
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.
================
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>
----------------
s/class/typename
================
Comment at: include/clang/AST/TextNodeDumper.h:199
+#include "clang/AST/AttrTextNodeDump.inc"
+
----------------
I'd appreciate some comments gently reminding the reader that this include
introduces declarations which are expected to be members of a class.
================
Comment at: lib/AST/ASTDumper.cpp:343
+
+#include "clang/AST/AttrNodeTraverse.inc"
};
----------------
Similarly, add some comments about how this adds implementations to the class.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:3714
+
+ std::string functionContent;
+ llvm::raw_string_ostream SS(functionContent);
----------------
`FunctionContent`
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:3726
+ functionContent = SS.str();
+ if (SS.tell()) {
+ OS << " void Visit" << R.getName() << "Attr(const " << R.getName()
----------------
Wouldn't this be `!FunctionContent.empty()`?
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:3747
- for (const auto *AI : Args)
- createArgument(*AI, R.getName())->writeDumpChildren(OS);
+ std::string functionContent;
+ llvm::raw_string_ostream SS(functionContent);
----------------
`FunctionContent`
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
