Extend the CommentVisitor with parameter types

[steveire]

Summary

This has precedent in the StmtVisitor. This change will make it
possible to clean up the comment handling in ASTDumper.

[aaron.ballman]

Overall this seems reasonable, but this change is currently a no-op because nothing is using this -- what's the plan for using and testing these changes?

include/clang/AST/CommentVisitor.h

31

Should this be ParamTys&&... because we're forwarding in the actual call? (Similar below)

[aaron.ballman]

In D55069#1313276, @aaron.ballman wrote:

Overall this seems reasonable, but this change is currently a no-op because nothing is using this -- what's the plan for using and testing these changes?

The answer is: D55070, which is NFC and presumably already has test coverage for it.

[steveire]

Add &&

[steveire]

I'm not aware of existing test coverage for any of this.

[steveire]

Add &&

[steveire]

Remove &&

[steveire]

The follow-up patch didn't build anymore with the &&.

../tools/clang/lib/AST/ASTDumper.cpp:2672:75: error: cannot bind rvalue reference of type ‘const clang::comments::FullComment*&&’ to lvalue of type ‘const clang::comments::FullComment* const’
     ConstCommentVisitor<ASTDumper, void, const FullComment *>::visit(C, FC);

[steveire]

Ah, test/Misc/ast-dump-comment.cpp is existing test coverage for this indeed, great.

[aaron.ballman]

In D55069#1313591, @steveire wrote:

The follow-up patch didn't build anymore with the &&.

../tools/clang/lib/AST/ASTDumper.cpp:2672:75: error: cannot bind rvalue reference of type ‘const clang::comments::FullComment*&&’ to lvalue of type ‘const clang::comments::FullComment* const’
     ConstCommentVisitor<ASTDumper, void, const FullComment *>::visit(C, FC);

Huh, that's surprising. It's a perfect forwarding reference and that use would be binding an lvalue reference, not an rvalue reference (I believe -- it's a bit hard to tell given that the patch with the uses is split out). Did you update both visit() and visitComment()? I just noticed that StmtVisitor also does not use perfect forwarding, so perhaps this isn't critical, but I'm curious as to why this doesn't behave as expected. One of the reasons I think this may be important is with the JSON dumper -- it may pass around JSON values rather than references and rely on move semantics to make this work.

[steveire]

Huh, that's surprising. It's a perfect forwarding reference

It's not a forwarding reference because the template parameter is from the record, not the function. See

https://godbolt.org/z/L4N2aS

One of the reasons I think this may be important is with the JSON dumper -- it may pass around JSON values rather than references and rely on move semantics to make this work.

I don't know what you're planning so I can't imagine what you want to pass through these visit function parameters. It would be surprising to me if you used this feature of the visitor for json dumping.

[aaron.ballman]

In D55069#1314508, @steveire wrote:

Huh, that's surprising. It's a perfect forwarding reference

It's not a forwarding reference because the template parameter is from the record, not the function. See

https://godbolt.org/z/L4N2aS

Ah, you're exactly right! I hadn't picked up on that.

One of the reasons I think this may be important is with the JSON dumper -- it may pass around JSON values rather than references and rely on move semantics to make this work.

I don't know what you're planning so I can't imagine what you want to pass through these visit function parameters. It would be surprising to me if you used this feature of the visitor for json dumping.

It can be dealt with if/when it arises.

LGTM!

include/clang/AST/CommentVisitor.h

 #include "llvm/Support/ErrorHandling.h"
 
 namespace clang {
 namespace comments {
 
 template <typename T> struct make_ptr { using type = T *; };
 template <typename T> struct make_const_ptr { using type = const T *; };
 
-template<template <typename> class Ptr, typename ImplClass, typename RetTy=void>
+template <template <typename> class Ptr, typename ImplClass,
+          typename RetTy = void, class... ParamTys>
 class CommentVisitorBase {
 public:
 #define PTR(CLASS) typename Ptr<CLASS>::type
-#define DISPATCH(NAME, CLASS) \
- return static_cast<ImplClass*>(this)->visit ## NAME(static_cast<PTR(CLASS)>(C))
+#define DISPATCH(NAME, CLASS)                                                  \
+  return static_cast<ImplClass *>(this)->visit##NAME(                          \
+      static_cast<PTR(CLASS)>(C), std::forward<ParamTys>(P)...)
 
-  RetTy visit(PTR(Comment) C) {
+  RetTy visit(PTR(Comment) C, ParamTys&&... P) {
     if (!C)
       return RetTy();
 
     switch (C->getCommentKind()) {
     default: llvm_unreachable("Unknown comment kind!");
 #define ABSTRACT_COMMENT(COMMENT)
 #define COMMENT(CLASS, PARENT) \
     case Comment::CLASS##Kind: DISPATCH(CLASS, CLASS);
 #include "clang/AST/CommentNodes.inc"
 #undef ABSTRACT_COMMENT
 #undef COMMENT
     }
   }
 
   // If the derived class does not implement a certain Visit* method, fall back
   // on Visit* method for the superclass.
 #define ABSTRACT_COMMENT(COMMENT) COMMENT
-#define COMMENT(CLASS, PARENT) \
-  RetTy visit ## CLASS(PTR(CLASS) C) { DISPATCH(PARENT, PARENT); }
+#define COMMENT(CLASS, PARENT)                                                 \
+  RetTy visit##CLASS(PTR(CLASS) C, ParamTys&&... P) { DISPATCH(PARENT, PARENT); }
 #include "clang/AST/CommentNodes.inc"
 #undef ABSTRACT_COMMENT
 #undef COMMENT
 
-  RetTy visitComment(PTR(Comment) C) { return RetTy(); }
+  RetTy visitComment(PTR(Comment) C, ParamTys&&... P) { return RetTy(); }
 
 #undef PTR
 #undef DISPATCH
 };
 
-template<typename ImplClass, typename RetTy=void>
-class CommentVisitor :
-    public CommentVisitorBase<make_ptr, ImplClass, RetTy> {};
-
-template<typename ImplClass, typename RetTy=void>
-class ConstCommentVisitor :
-    public CommentVisitorBase<make_const_ptr, ImplClass, RetTy> {};
+template <typename ImplClass, typename RetTy = void, class... ParamTys>
+class CommentVisitor
+    : public CommentVisitorBase<make_ptr, ImplClass, RetTy, ParamTys...> {};
+
+template <typename ImplClass, typename RetTy = void, class... ParamTys>
+class ConstCommentVisitor
+    : public CommentVisitorBase<make_const_ptr, ImplClass, RetTy, ParamTys...> {
+};
 
 } // namespace comments
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_COMMENTVISITOR_H