Allow any comment to be a trailing comment when -fparse-all-comments is on.

[zarko]

Summary

This helps with freeform documentation styles, where otherwise code like

enum class E {
  E1,  // D1
  E2   // D2
};

would result in D1 being associated with E2. To properly associate E1
with D1 and E2 with D2, this patch allows all raw comments C such that
C.isParseAllComments() to participate in trailing comment checks inside
getRawCommentForDeclNoCache. This takes care of linking the intended
documentation with the intended decls. There remains an issue with code
like:

foo();  // DN
int x;

To prevent DN from being associated with x, this patch adds a new test
on preceding-line comments C (where C.isParseAllComments() and also
C's kind is RCK_OrdinaryBCPL or RCK_OrdinaryC) that checks whether C
is the first non-whitespace thing on C's starting line.

[rsmith]

lib/AST/ASTContext.cpp

70

Please start this with a lowercase letter.

233–234

If the comment is a documentation comment but doesn't use the trailing comment syntax, what does your patch do? For instance:

enum E {
  A,  /// Comment about A or B?
  B
};

The comment is recognized and attached to B outside parse-all-comments mode, so it should presumably mean the same thing in parse-all-comments mode, as broken as that is.

[zarko]

Keep logic in ASTContext the same; instead infer whether ordinary comments are trailing ones.

lib/AST/ASTContext.cpp

233–234

As the patch was written, the documentation would get attached to both A and B. I've changed strategies and now try to guess whether an ordinary comment is trailing or not (and whether that should be merged); behavior for cases like the one you've indicated is documented and checked in parse-all-comments.c.

[rsmith]

lib/AST/RawCommentList.cpp

77–88 ↗

(On Diff #29473)

Use clang::isHorizontalWhitespace / clang::isVerticalWhitespace instead (from Basic/CharInfo.h).

300–302 ↗

(On Diff #29473)

I find this a bit worrying...

test/Index/parse-all-comments.c

42–44

... because this case probably shouldn't get merged if the second comment is not indented. It seems that the indentation is the only way we can tell that case apart from:

int trdoxyB;  // Not a Doxygen trailing comment.
// This documents trdoxyC, not trdoxyB.
​int trdoxyC;

Perhaps we should only allow ordinary comments to merge with trailing comments if they start in the same column. I'm not too happy about making the semantics depend on the indentation, but you're already proposing that they depend on whether there are non-whitespace characters earlier on the same line, so perhaps that is OK.

[zarko]

Only merge ordinary comments if they're trailing and start on the same column as the comment we would merge them into.

[rsmith]

LGTM

lib/AST/RawCommentList.cpp

96 ↗

(On Diff #29729)

No else after return. I'd also drop the braces here.

311–312 ↗

(On Diff #29729)

Maybe indent two spaces more inside these two comments?

[zarko]

Address style comments.

[jdennett]

Committed as r242317.

lib/AST/ASTContext.cpp

 unsigned ASTContext::NumImplicitMoveAssignmentOperatorsDeclared;
 unsigned ASTContext::NumImplicitDestructors;
 unsigned ASTContext::NumImplicitDestructorsDeclared;
 
 enum FloatingRank {
   HalfRank, FloatRank, DoubleRank, LongDoubleRank
 };
 
+/// \brief Determines whether there is only whitespace in `Buffer` between `P`
+/// and the previous line.
+/// \param Buffer The buffer to search in.
+/// \param P The offset from the beginning of `Buffer` to start from.
+/// \return true if all of the characters in `Buffer` ranging from the closest
+/// line-ending character before `P` (or the beginning of `Buffer`) to `P - 1`
+/// are whitespace.
+static bool OnlyWhitespaceOnLineBefore(const char *Buffer, unsigned P) {
+  // Search backwards until we see linefeed or carriage return.
+  for (unsigned I = P; I != 0; --I) {
+    switch (Buffer[I - 1]) {
+    default:
+      return false;
+    case ' ':
+    case '\t':
+    case '\f':
+    case '\v':
+      break;
+    case '\r':
+    case '\n':
+      return true;
+    }
+  }
+  // We hit the beginning of the buffer.
+  return true;
+}
+
 RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
   if (!CommentsLoaded && ExternalSource) {
     ExternalSource->ReadComments();
 
 #ifndef NDEBUG
     ArrayRef<RawComment *> RawComments = Comments.getComments();
     assert(std::is_sorted(RawComments.begin(), RawComments.end(),
                           BeforeThanCompare<RawComment>(SourceMgr)));
#endif
   }
 
   // Decompose the location for the declaration and find the beginning of the
   // file buffer.
   std::pair<FileID, unsigned> DeclLocDecomp = SourceMgr.getDecomposedLoc(DeclLoc);
 
   // First check whether we have a trailing comment.
   if (Comment != RawComments.end() &&
-      (*Comment)->isDocumentation() && (*Comment)->isTrailingComment() &&
+      ((*Comment)->isParseAllComments() ||
+       ((*Comment)->isDocumentation() && (*Comment)->isTrailingComment())) &&
       (isa<FieldDecl>(D) || isa<EnumConstantDecl>(D) || isa<VarDecl>(D) ||
        isa<ObjCMethodDecl>(D) || isa<ObjCPropertyDecl>(D))) {
     std::pair<FileID, unsigned> CommentBeginDecomp
       = SourceMgr.getDecomposedLoc((*Comment)->getSourceRange().getBegin());
     // Check that Doxygen trailing comment comes after the declaration, starts
     // on the same line and in the same file as the declaration.
     if (DeclLocDecomp.first == CommentBeginDecomp.first &&
         SourceMgr.getLineNumber(DeclLocDecomp.first, DeclLocDecomp.second)
           == SourceMgr.getLineNumber(CommentBeginDecomp.first,
                                      CommentBeginDecomp.second)) {
       return *Comment;
     }
   }
 
   // The comment just after the declaration was not a trailing comment.
   // Let's look at the previous comment.
   if (Comment == RawComments.begin())
     return nullptr;
   --Comment;
 
+  if ((*Comment)->isParseAllComments() &&
+      ((*Comment)->getKind() == RawComment::RCK_OrdinaryBCPL ||
+       (*Comment)->getKind() == RawComment::RCK_OrdinaryC)) {
+    // When we're trying to guess which comments are actually documentation
+    // comments and the previous comment was ordinary, assume that it's
+    // unrelated if it's not the first thing on its starting line. This may
+    // be a comment unrelated to anything, or it might be a trailing comment
+    // for a different Decl.
+    std::pair<FileID, unsigned> CommentBeginDecomp =
+        SourceMgr.getDecomposedLoc((*Comment)->getSourceRange().getBegin());
+    if (CommentBeginDecomp.second != 0) {
+      // We're spuriously OK if this was the first thing in the file.
+      bool Invalid = false;
+      const char *Buffer =
+          SourceMgr.getBufferData(CommentBeginDecomp.first, &Invalid).data();
+      if (Invalid ||
+          !OnlyWhitespaceOnLineBefore(Buffer, CommentBeginDecomp.second))
+        return nullptr;
+    }
+  }
+
   // Check that we actually have a non-member Doxygen comment.
   if (!(*Comment)->isDocumentation() || (*Comment)->isTrailingComment())
     return nullptr;
 
   // Decompose the end of the comment.
   std::pair<FileID, unsigned> CommentEndDecomp
     = SourceMgr.getDecomposedLoc((*Comment)->getSourceRange().getEnd());
 

test/Index/parse-all-comments.c

 /** But there are other blocks that are part of the comment, too.  IS_DOXYGEN_END */
 void multi_line_comment_plus_ordinary(int);
 
 // MULTILINE COMMENT
 //
 // WITH EMPTY LINE
 void multi_line_comment_empty_line(int);
 
+int notdoxy7; // Not a Doxygen juxtaposed comment.  notdoxy7 NOT_DOXYGEN
+int notdoxy8; // Not a Doxygen juxtaposed comment.  notdoxy8 NOT_DOXYGEN
+
 #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:19:6: FunctionDecl=isdoxy5:{{.*}} isdoxy5 IS_DOXYGEN_SINGLE
 // CHECK: parse-all-comments.c:22:6: FunctionDecl=isdoxy6:{{.*}} isdoxy6 IS_DOXYGEN_SINGLE
 // CHECK: parse-all-comments.c:29:6: FunctionDecl=multi_line_comment_plus_ordinary:{{.*}} BLOCK_ORDINARY_COMMENT {{.*}} ORDINARY COMMENT {{.*}} IS_DOXYGEN_START {{.*}} IS_DOXYGEN_END
 // CHECK: parse-all-comments.c:34:6: FunctionDecl=multi_line_comment_empty_line:{{.*}} MULTILINE COMMENT{{.*}}\n{{.*}}\n{{.*}} WITH EMPTY LINE
+// CHECK: parse-all-comments.c:36:5: VarDecl=notdoxy7:{{.*}} notdoxy7 NOT_DOXYGEN
+// CHECK: parse-all-comments.c:37:5: VarDecl=notdoxy8:{{.*}} notdoxy8 NOT_DOXYGEN