This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by zarko on Jul 9 2015, 10:50 AM.

Details

Reviewers
rsmith
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.

Diff Detail

Event Timeline

zarko updated this revision to Diff 29357.Jul 9 2015, 10:50 AM
zarko retitled this revision from to Allow any comment to be a trailing comment when -fparse-all-comments is on..
zarko updated this object.
zarko added a subscriber: cfe-commits.
rsmith added a subscriber: rsmith.Jul 9 2015, 4:42 PM
rsmith added inline comments.
lib/AST/ASTContext.cpp
70 ↗(On Diff #29357)

Please start this with a lowercase letter.

233–234 ↗(On Diff #29357)

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 updated this revision to Diff 29473.Jul 10 2015, 12:30 PM
zarko marked an inline comment as done.

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

lib/AST/ASTContext.cpp
233–234 ↗(On Diff #29357)

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 added inline comments.Jul 14 2015, 2:54 PM
lib/AST/RawCommentList.cpp
77–88

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

300–302

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 updated this revision to Diff 29729.Jul 14 2015, 4:23 PM
zarko marked 3 inline comments as done.

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

rsmith accepted this revision.Jul 14 2015, 4:59 PM
rsmith added a reviewer: rsmith.

LGTM

lib/AST/RawCommentList.cpp
96

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

311–312

Maybe indent two spaces more inside these two comments?

This revision is now accepted and ready to land.Jul 14 2015, 4:59 PM
zarko updated this revision to Diff 29735.Jul 14 2015, 5:07 PM
zarko edited edge metadata.
zarko marked an inline comment as done.

Address style comments.

zarko marked an inline comment as done.Jul 14 2015, 5:09 PM
jdennett closed this revision.Jul 15 2015, 12:14 PM
jdennett added a subscriber: jdennett.

Committed as r242317.