This change-list changes clang to attach the ordinary comments if an option is specified

[amshali]

Summary

Not all people use doxygen style comments as doc-comments. They just use ordinary comments in place of doxygen style comments. We would like to know which nodes are associated with those comments.

[gribozavr]

I think a name like "parse all comments" would better reflect the intent of this feature.

include/clang/AST/RawCommentList.h

13–15

Please sort includes.

92

It would be better to change isOrdinary instead:

return (Kind == RCK_OrdinaryBCPL) || (Kind == RCK_OrdinaryC) || TreatOrdinaryCommentAsDocComment;

144

Only "//"? It would be better to rename it to "parse all comments". Is this OK with you?

145

Three slashes, please.

145

And please spell 'documentation comments' completely.

146

Please pack this at the end of the bitfield above (after IsTrailingComment, IsAlmostTrailingComment).

lib/AST/RawCommentList.cpp

258

I don't think the last 'true' is necessary here. Why do you think it is?

unittests/Frontend/FrontendActionTest.cpp

103–104 ↗

(On Diff #1486)

Star goes to the right. Member names should start with a capital letter and should not have an underscore.

108–109 ↗

(On Diff #1486)

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.

112–113 ↗

(On Diff #1486)

Please add tests for /* ... */ comments.

[amshali]

Changes in this revision:

• Applied reviewer's comments.
• Renamed the option to ParseAllComments.
• Fixed the code appropriately.
• Removed the test from unittests/Frontend/FrontendActionTest.cpp. * Added two tests, one for making sure we are passing the argument to clang and one for making sure we are attaching ordinary comments.
• Changed Tools.cpp to pass the option to cc1.
• Changed the option definition in Options.td.

[amshali]

Please have a look. I added some new tests and applied most of your comments.

include/clang/AST/RawCommentList.h

13–15

Done.

92

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.

144

Sure. Done.

145

Done.

145

Done.

146

Done.

lib/AST/RawCommentList.cpp

258

'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?

unittests/Frontend/FrontendActionTest.cpp

103–104 ↗

(On Diff #1486)

Done.

108–109 ↗

(On Diff #1486)

didn't know about that. Thanks.

[amshali]

test/Index/parse-all-comments.c

36

For some reason the out.c-index-pch does not have the I expected.
Running the follwing check fails after this RUN:
// RUN: c-index-test -test-load-tu %t/out.pch all > %t/out.c-index-pch

Can you help me with that? Is it necessary to run the check on out.c-index-pch or out.c-index-direct good enough?

[amshali]

Hi
No rush, but I was wondering if you or someone could take another look at this. Thank you.

[gribozavr]

Please also mention this new command line option in the documentation. http://clang.llvm.org/docs/UsersManual.html should be a good place. Add a new section about documentation comments if this does not fall naturally into existing sections.

include/clang/AST/RawCommentList.h

92

Yes, that line is incorrect. The correct should be:

return ((Kind == RCK_OrdinaryBCPL) || (Kind == RCK_OrdinaryC)) && !ParseAllComments;

Does this fix the issue in ASTContext?

144

Also mention /* ... */ comments.

include/clang/Basic/CommentOptions.h

31–32

Serialization/deserialization code is missing. Grep for BlockCommandNames to find the relevant places.

lib/AST/RawCommentList.cpp

232

Why this change? isOrdinary() should be the exact opposite of isDocumentation() (modulo invalid cases).

test/Index/parse-all-comments.c

7–8

Please add a test that consecutive '//' comments are merged. See isdoxy20 in the original test for an idea.

36

Yes, pch usecase is important. This error is caused by missing serialization/deserialization code in lib/Serialization/ASTReader.cpp and lib/Serialization/ASTWriter.cpp.

[amshali]

Applied comments. Fixed isOrdinary and isDocumentation comments with the right logic for parsing all comments. Reverted the change in RawCommentList.cpp regarding isOrdinary. Added some new tests. Added the pch tests. Updated user manual with a section for comment processing options. Fixed up the serialization code for comment options.

[amshali]

Thank you for comments and help. Updated UsersManual.rst with a new section for comment processing options. Please take a look.

include/clang/AST/RawCommentList.h

92

Since isDocumentation is the opposite of isOrdinary, I need to change both for whole thing to work fine.

144

Done.

include/clang/Basic/CommentOptions.h

31–32

Done. Thanks.

lib/AST/RawCommentList.cpp

232

Reverted. Fixed isOrdinary and isDocumentation. PTAL.

test/Index/parse-all-comments.c

7–8

Added.

36

Thanks. Done.

[gribozavr]

This patch looks good, I have just a few style remarks.

docs/UsersManual.rst

1318

Please spell Doxygen with a capital 'D'.

1325–1326

Maybe:

Clang will parse all comments as documentation comments (including ordinary comments starting with `//` and `/*`).

include/clang/AST/RawCommentList.h

44–45

I think these default arguments are unused now. If they are, please remove them.

87–88

LLVM and Clang style is to leave '&&' on the previous line.

145–147

Please put two slashes on the second line (to turn this into a documentation comment). Also put a blank line before this comment.

include/clang/Basic/CommentOptions.h

31–32

Please spell 'documentation comments' completely.

[amshali]

Removed default parameter from RawComment constructor. Fixed users manual text. Fixed comments. Fixed the if condition style.

[amshali]

Thanks. Applied your comments. Let me know what should I do next.

docs/UsersManual.rst

1318

Done.

1325–1326

Done.

include/clang/AST/RawCommentList.h

44–45

Done.

87–88

Done.

145–147

Done.

include/clang/Basic/CommentOptions.h

31–32

Done.

[gribozavr]

Committed r179180, thank you!

docs/UsersManual.rst

 
 `Some tests might fail <http://llvm.org/bugs/show_bug.cgi?id=9072>`_ on
 ``x86_64-w64-mingw32``.
+
+Comment Processing Options
+==========================
+
+Clang parses Doxygen and non-Doxygen style documentation comments and attaches
+them to the appropriate declaration nodes. By default, it only parses Doxygen
+style comments and ignores ordinary comments starting with `//` and `/*`.
+The following option will change this default behavior:
+
+.. option:: -fparse-all-comments
+
+  Clang will parse all comments as documentation comments (including ordinary 
+  comments starting with `//` and `/*`).

include/clang/AST/RawCommentList.h

 #ifndef LLVM_CLANG_AST_RAW_COMMENT_LIST_H
 #define LLVM_CLANG_AST_RAW_COMMENT_LIST_H
 
+#include "clang/Basic/CommentOptions.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/ArrayRef.h"
 

   RawComment() : Kind(RCK_Invalid), IsAlmostTrailingComment(false) { }
 
   RawComment(const SourceManager &SourceMgr, SourceRange SR,
-             bool Merged = false);
+             bool Merged, bool ParseAllComments);
 
   CommentKind getKind() const LLVM_READONLY {
     return (CommentKind) Kind;

 
   /// Returns true if this comment is not a documentation comment.
   bool isOrdinary() const LLVM_READONLY {
-    return (Kind == RCK_OrdinaryBCPL) || (Kind == RCK_OrdinaryC);
+    return ((Kind == RCK_OrdinaryBCPL) || (Kind == RCK_OrdinaryC)) &&
+        !ParseAllComments;
   }
 
   /// Returns true if this comment any kind of a documentation comment.
   bool isDocumentation() const LLVM_READONLY {
-    return !isInvalid() && !isOrdinary();
+    return !isInvalid() && (!isOrdinary() || ParseAllComments);
+  }
+
+  /// Returns whether we are parsing all comments.
+  bool isParseAllComments() const LLVM_READONLY {
+    return ParseAllComments;
   }
 
   /// Returns raw comment text with comment markers.

   bool IsTrailingComment : 1;
   bool IsAlmostTrailingComment : 1;
 
+  /// When true, ordinary comments starting with "//" and "/*" will be
+  /// considered as documentation comments.
+  bool ParseAllComments : 1;
+
   mutable bool BeginLineValid : 1; ///< True if BeginLine is valid
   mutable bool EndLineValid : 1;   ///< True if EndLine is valid
   mutable unsigned BeginLine;      ///< Cached line number

 
   /// \brief Constructor for AST deserialization.
   RawComment(SourceRange SR, CommentKind K, bool IsTrailingComment,
-             bool IsAlmostTrailingComment) :
+             bool IsAlmostTrailingComment,
+             bool ParseAllComments) :
     Range(SR), RawTextValid(false), BriefTextValid(false), Kind(K),
     IsAttached(false), IsTrailingComment(IsTrailingComment),
     IsAlmostTrailingComment(IsAlmostTrailingComment),
+    ParseAllComments(ParseAllComments),
     BeginLineValid(false), EndLineValid(false)
   { }
 

 } // end namespace clang
 
 #endif
-

include/clang/Basic/CommentOptions.h

   /// \brief Command names to treat as block commands in comments.
   /// Should not include the leading backslash.
   BlockCommandNamesTy BlockCommandNames;
+
+  /// \brief Treat ordinary comments as documentation comments.
+  bool ParseAllComments;
+
+  CommentOptions() : ParseAllComments(false) { }
 };
 
 }  // end namespace clang

include/clang/Driver/Options.td

 def fcomment_block_commands : CommaJoined<["-"], "fcomment-block-commands=">, Group<f_clang_Group>, Flags<[CC1Option]>,
   HelpText<"Treat each comma separated argument in <arg> as a documentation comment block command">,
   MetaVarName<"<arg>">;
+def fparse_all_comments : Flag<["-"], "fparse-all-comments">, Group<f_clang_Group>, Flags<[CC1Option]>;
 def fcommon : Flag<["-"], "fcommon">, Group<f_Group>;
 def fcompile_resource_EQ : Joined<["-"], "fcompile-resource=">, Group<f_Group>;
 def fconstant_cfstrings : Flag<["-"], "fconstant-cfstrings">, Group<f_Group>;

lib/AST/ASTContext.cpp

     // When searching for comments during parsing, the comment we are looking
     // for is usually among the last two comments we parsed -- check them
     // first.
-    RawComment CommentAtDeclLoc(SourceMgr, SourceRange(DeclLoc));
+    RawComment CommentAtDeclLoc(
+        SourceMgr, SourceRange(DeclLoc), false,
+        LangOpts.CommentOpts.ParseAllComments);
     BeforeThanCompare<RawComment> Compare(SourceMgr);
     ArrayRef<RawComment *>::iterator MaybeBeforeDecl = RawComments.end() - 1;
     bool Found = Compare(*MaybeBeforeDecl, &CommentAtDeclLoc);

lib/AST/RawCommentList.cpp

 } // unnamed namespace
 
 RawComment::RawComment(const SourceManager &SourceMgr, SourceRange SR,
-                       bool Merged) :
+                       bool Merged, bool ParseAllComments) :
     Range(SR), RawTextValid(false), BriefTextValid(false),
     IsAttached(false), IsAlmostTrailingComment(false),
+    ParseAllComments(ParseAllComments),
     BeginLineValid(false), EndLineValid(false) {
   // Extract raw comment text, if possible.
   if (SR.getBegin() == SR.getEnd() || getRawText(SourceMgr).empty()) {

     if (C1EndLine + 1 == C2BeginLine || C1EndLine == C2BeginLine) {
       SourceRange MergedRange(C1.getSourceRange().getBegin(),
                               C2.getSourceRange().getEnd());
-      *Comments.back() = RawComment(SourceMgr, MergedRange, true);
+      *Comments.back() = RawComment(SourceMgr, MergedRange, true,
+                                    RC.isParseAllComments());
       Merged = true;
     }
   }

 
   OnlyWhitespaceSeen = true;
 }
-

lib/Driver/Tools.cpp

 
   // Forward -fcomment-block-commands to -cc1.
   Args.AddAllArgs(CmdArgs, options::OPT_fcomment_block_commands);
+  // Forward -fparse-all-comments to -cc1.
+  Args.AddAllArgs(CmdArgs, options::OPT_fparse_all_comments);
 
   // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
   // parser.

lib/Frontend/CompilerInvocation.cpp

 
 static void ParseCommentArgs(CommentOptions &Opts, ArgList &Args) {
   Opts.BlockCommandNames = Args.getAllArgValues(OPT_fcomment_block_commands);
+  Opts.ParseAllComments = Args.hasArg(OPT_fparse_all_comments);
 }
 
 static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,

lib/Sema/Sema.cpp

   if (!LangOpts.RetainCommentsFromSystemHeaders &&
       SourceMgr.isInSystemHeader(Comment.getBegin()))
     return;
-  RawComment RC(SourceMgr, Comment);
+  RawComment RC(SourceMgr, Comment, false,
+                LangOpts.CommentOpts.ParseAllComments);
   if (RC.isAlmostTrailingComment()) {
     SourceRange MagicMarkerRange(Comment.getBegin(),
                                  Comment.getBegin().getLocWithOffset(3));

lib/Serialization/ASTReader.cpp

     LangOpts.CommentOpts.BlockCommandNames.push_back(
       ReadString(Record, Idx));
   }
+  LangOpts.CommentOpts.ParseAllComments = Record[Idx++];
 
   return Listener.ReadLanguageOptions(LangOpts, Complain);
 }

             (RawComment::CommentKind) Record[Idx++];
         bool IsTrailingComment = Record[Idx++];
         bool IsAlmostTrailingComment = Record[Idx++];
-        Comments.push_back(new (Context) RawComment(SR, Kind,
-                                                    IsTrailingComment,
-                                                    IsAlmostTrailingComment));
+        Comments.push_back(new (Context) RawComment(
+            SR, Kind, IsTrailingComment, IsAlmostTrailingComment,
+            Context.getLangOpts().CommentOpts.ParseAllComments));
         break;
       }
       }

lib/Serialization/ASTWriter.cpp

        I != IEnd; ++I) {
     AddString(*I, Record);
   }
+  Record.push_back(LangOpts.CommentOpts.ParseAllComments);
 
   Stream.EmitRecord(LANGUAGE_OPTIONS, Record);
 

test/Driver/fparse-all-comments.c

+// Check that we pass -fparse-all-comments to frontend.
+//
+// RUN: %clang -c %s -fparse-all-comments -### 2>&1 | FileCheck %s --check-prefix=CHECK-ARG
+//
+// CHECK-ARG: -fparse-all-comments

test/Index/parse-all-comments.c

+// Run lines are sensitive to line numbers and come below the code.
+
+#ifndef HEADER
+#define HEADER
+
+// Not a Doxygen comment.  notdoxy1 NOT_DOXYGEN
+void notdoxy1(void);
+
+/* Not a Doxygen comment.  notdoxy2 NOT_DOXYGEN */
+void notdoxy2(void);
+
+/*/ Not a Doxygen comment.  notdoxy3 NOT_DOXYGEN */
+void notdoxy3(void);
+
+/** Doxygen comment.  isdoxy4 IS_DOXYGEN_SINGLE */
+void isdoxy4(void);
+
+/* BLOCK_ORDINARY_COMMENT */
+// ORDINARY COMMENT
+/// This is a BCPL comment. IS_DOXYGEN_START
+/// It has only two lines.
+/** But there are other blocks that are part of the comment, too.  IS_DOXYGEN_END */
+void multi_line_comment_plus_ordinary(int);
+
+#endif
+
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// RUN: %clang_cc1 -fparse-all-comments -x c++ -std=c++11 -emit-pch -o %t/out.pch %s
+
+// RUN: c-index-test -test-load-source all -comments-xml-schema=%S/../../bindings/xml/comment-xml-schema.rng %s -std=c++11 -fparse-all-comments > %t/out.c-index-direct
+// RUN: c-index-test -test-load-tu %t/out.pch all > %t/out.c-index-pch
+
+// RUN: FileCheck %s -check-prefix=WRONG < %t/out.c-index-direct
+// RUN: FileCheck %s -check-prefix=WRONG < %t/out.c-index-pch
+
+// Ensure that XML is not invalid
+// WRONG-NOT: CommentXMLInvalid
+
+// RUN: FileCheck %s < %t/out.c-index-direct
+// RUN: FileCheck %s < %t/out.c-index-pch
+
+// CHECK: parse-all-comments.c:7:6: FunctionDecl=notdoxy1:{{.*}} notdoxy1 NOT_DOXYGEN
+// CHECK: parse-all-comments.c:10:6: FunctionDecl=notdoxy2:{{.*}} notdoxy2 NOT_DOXYGEN
+// CHECK: parse-all-comments.c:13:6: FunctionDecl=notdoxy3:{{.*}} notdoxy3 NOT_DOXYGEN
+// CHECK: parse-all-comments.c:16:6: FunctionDecl=isdoxy4:{{.*}} isdoxy4 IS_DOXYGEN_SINGLE
+// CHECK: parse-all-comments.c:23:6: FunctionDecl=multi_line_comment_plus_ordinary:{{.*}} BLOCK_ORDINARY_COMMENT {{.*}} ORDINARY COMMENT {{.*}} IS_DOXYGEN_START {{.*}} IS_DOXYGEN_END