[PATCH] This change-list changes clang to attach the ordinary comments if an option is specified
Amin Shali
amshali at gmail.com
Wed Apr 3 16:00:19 PDT 2013
Please have a look. I added some new tests and applied most of your comments.
================
Comment at: include/clang/AST/RawCommentList.h:13-15
@@ -12,4 +12,5 @@
#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/CommentOptions.h"
#include "llvm/ADT/ArrayRef.h"
----------------
Dmitri Gribenko wrote:
> Please sort includes.
Done.
================
Comment at: include/clang/AST/RawCommentList.h:92
@@ -89,3 +91,3 @@
bool isDocumentation() const LLVM_READONLY {
- return !isInvalid() && !isOrdinary();
+ return !isInvalid() && (!isOrdinary() || TreatOrdinaryCommentAsDocComment);
}
----------------
Dmitri Gribenko wrote:
> It would be better to change isOrdinary instead:
>
> return (Kind == RCK_OrdinaryBCPL) || (Kind == RCK_OrdinaryC) || TreatOrdinaryCommentAsDocComment;
>
Wait, you mean:
return (Kind == RCK_OrdinaryBCPL) || (Kind == RCK_OrdinaryC) || !ParseAllComments;
But then it skips doxygen style comments when ParseAllComments is false.
Besides I think it should be in isDocumentation because otherwise it will fail to return the comment for a declaration because of the check at line 191 file: ASTContext.cpp.
================
Comment at: include/clang/AST/RawCommentList.h:144
@@ -141,1 +143,3 @@
mutable unsigned EndLine; ///< Cached line number
+ /// When true, ordinary comments starting with "//" will be considered as
+ // doc-comments.
----------------
Dmitri Gribenko wrote:
> Only "//"? It would be better to rename it to "parse all comments". Is this OK with you?
>
Sure. Done.
================
Comment at: include/clang/AST/RawCommentList.h:145
@@ +144,3 @@
+ /// When true, ordinary comments starting with "//" will be considered as
+ // doc-comments.
+ bool TreatOrdinaryCommentAsDocComment;
----------------
Dmitri Gribenko wrote:
> Dmitri Gribenko wrote:
> > Three slashes, please.
> And please spell 'documentation comments' completely.
Done.
================
Comment at: include/clang/AST/RawCommentList.h:145
@@ +144,3 @@
+ /// When true, ordinary comments starting with "//" will be considered as
+ // doc-comments.
+ bool TreatOrdinaryCommentAsDocComment;
----------------
Amin Shali wrote:
> Dmitri Gribenko wrote:
> > Dmitri Gribenko wrote:
> > > Three slashes, please.
> > And please spell 'documentation comments' completely.
> Done.
Done.
================
Comment at: include/clang/AST/RawCommentList.h:146
@@ -142,1 +145,3 @@
+ // doc-comments.
+ bool TreatOrdinaryCommentAsDocComment;
----------------
Dmitri Gribenko wrote:
> Please pack this at the end of the bitfield above (after IsTrailingComment, IsAlmostTrailingComment).
>
Done.
================
Comment at: unittests/Frontend/FrontendActionTest.cpp:103-104
@@ +102,4 @@
+ private:
+ std::vector<std::string>* comments_;
+ ASTContext* context_;
+ };
----------------
Dmitri Gribenko wrote:
> Star goes to the right. Member names should start with a capital letter and should not have an underscore.
Done.
================
Comment at: unittests/Frontend/FrontendActionTest.cpp:108-109
@@ +107,4 @@
+
+TEST(ASTFrontendAction, TreatOrdinaryCommentAsDoccomment) {
+ CompilerInvocation *invocation = new CompilerInvocation;
+ invocation->getPreprocessorOpts().addRemappedFile(
----------------
Dmitri Gribenko wrote:
> Why is this test so complicated, going through constructing half of Clang?
>
> You can add a very simple test, based on c-index-test, see test/Index/annotate-comments.cpp.
didn't know about that. Thanks.
================
Comment at: lib/AST/RawCommentList.cpp:258
@@ -255,3 +257,3 @@
C2.getSourceRange().getEnd());
- *Comments.back() = RawComment(SourceMgr, MergedRange, true);
+ *Comments.back() = RawComment(SourceMgr, MergedRange, true, true);
Merged = true;
----------------
Dmitri Gribenko wrote:
> I don't think the last 'true' is necessary here. Why do you think it is?
>
'true' is not necessary, you are right. But 'false' is not correct either. I changed it so that it gets the correct value from RC. I think that should be correct. What do you think?
http://llvm-reviews.chandlerc.com/D614
More information about the cfe-commits
mailing list