[PATCH] D46000: [AST] Added a helper to extract a user-friendly text of a comment.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: lib/AST/CommentLexer.cpp:471
+      case '\r':
+        TokenPtr = skipNewline(TokenPtr, CommentEnd);
+        formTokenWithChars(T, TokenPtr, tok::newline);
----------------
ioeric wrote:
> Can we share code with `lexCommentTextWithCommands` for these two common cases?
I couldn't come up with a way to do that previsouly.
Made another attempt which seems to work.
Please take a look, the change is somewhat non-trivial (includes removing the loop that seems redundant)


================
Comment at: lib/AST/RawCommentList.cpp:380
+        SourceMgr.getSpellingColumnNumber(Tok.getLocation(), &LocInvalid);
+    if (LocInvalid)
+      TokColumn = 0;
----------------
ioeric wrote:
> Explain when this would be invalid and why `TokColumn = 0` is used?
I don't know whether this can be even be invalid, but I'm not confident enough to add an assert there.
`TokColumn = 0` seems like a reasonable way to recover if we can't compute the column number, i.e. assume the line starts at the first column if SourceLocation of the line was invalid for any reason.

This whole column thing looks weird to me, maybe I should just remove it altogether and just remove the same amount of whitespace in all the lines. WDYT?


================
Comment at: lib/AST/RawCommentList.cpp:383
+    // Compute the length of whitespace we're allowed to skip.
+    unsigned MaxSkip;
+    if (IsFirstLine) {
----------------
ioeric wrote:
> nit: `unsigned MaxSkip = IsFirstLine ? ... : ...;`
That would force to get rid of the comments in the if branches, but they seem to be useful.
Am I missing an obvious style that would preserve the comments?


================
Comment at: lib/AST/RawCommentList.cpp:392
+    }
+    llvm::StringRef Trimmed = SkipWs(TokText, MaxSkip);
+    Result += Trimmed;
----------------
ioeric wrote:
> I'd probably make `SkipWs` return the number of white spaces skipped and do the drop-front here, so that you could simplify the awkward calculation of `IndentColumn` below.
Got rid of it altogether.
The code seems is clearer now, thanks for the suggestion!




Repository:
  rC Clang

https://reviews.llvm.org/D46000