[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 llvm-commits mailing list