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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to