This is an archive of the discontinued LLVM Phabricator instance.

Extend the CommentVisitor with parameter types
ClosedPublic

Authored by steveire on Nov 29 2018, 11:35 AM.

Details

Summary

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

Diff Detail

Event Timeline

steveire created this revision.Nov 29 2018, 11:35 AM

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)

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.

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

steveire updated this revision to Diff 175967.Nov 29 2018, 2:34 PM

Remove &&

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);

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

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.

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 accepted this revision.Nov 30 2018, 6:10 AM

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!

This revision is now accepted and ready to land.Nov 30 2018, 6:10 AM
This revision was automatically updated to reflect the committed changes.