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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: include/clang/AST/RawCommentList.h:118
+  ///     //   Parts of it  might be indented.
+  ///     /* The comments styles might be mixed. */
+  ///  into
----------------
ioeric wrote:
> I'm trying to understand how these cases and RawComment work.
> 
> For this case, are the `// ... ` block and `/* ... */` merged in one `RawComment` by default?
Yes, `RawComment` can represent multiple merged comments of different styles.


================
Comment at: include/clang/AST/RawCommentList.h:126
+  ///      *   This is a second line. It is indented.
+  ///      * This is a third line. */
+  /// and
----------------
ioeric wrote:
> Are the `*`s in each lines automatically consumed by the lexer?
Yes, see `comments::Lexer::skipLineStartingDecorations()`


================
Comment at: lib/AST/RawCommentList.cpp:380
+        SourceMgr.getSpellingColumnNumber(Tok.getLocation(), &LocInvalid);
+    if (LocInvalid)
+      TokColumn = 0;
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > 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?
> > On a second thought, now I remember why I added this in the first place.
> > To support the following example we want to take column numbers into account:
> > 
> > ```
> > class Foo {
> >     /* A block comment
> >        spanning multiple lines
> >        has too many spaces on the all lines except the first one.
> >     */
> >     int func();
> > };
> > ```
> So I think you would want to force `MaxSkip` to 0 if token loc is invalid to make sure no comment is accidentally eaten?
Makes sense, thanks. Unfortunately I don't have any ideas on how we can test this case :-(


================
Comment at: lib/AST/RawCommentList.cpp:343
+  if (CommentText.empty())
+    return ""; // we couldn't retreive the comment.
+
----------------
ioeric wrote:
> nit: We don't really know if there was a failure or the comment is simply empty. So I'd probably leave out the comment here to avoid confusion.
`getRawTextSlow` returns empty string on error, so this comment has some ground.
Removed it anyway


================
Comment at: lib/AST/RawCommentList.cpp:349
+                    CommentText.begin(), CommentText.end(),
+                    /*ParseCommentText=*/false);
+
----------------
ioeric wrote:
> `s/ParseCommentText/ParseCommands/`?
Thanks for catching this!


================
Comment at: lib/AST/RawCommentList.cpp:393
+
+    llvm::StringRef Trimmed = TokText.drop_front(std::min(MaxSkip, WhitespaceLen));
+    Result += Trimmed;
----------------
ioeric wrote:
> I think `MaxSkip` could be removed with:
> 
> ```
> llvm::StringRef Trimmed = TokText.drop_front(IsFirstLine ? WhitespaceLen : 
>                                                  std::max((int)IndentColumn - (int)TokColumn, 0)));
> ```
I've removed `MaxSkip`, but introduced `SkipLen` instead that captures an actual numbers of chars we want to skip.
I would still keep as a separate variable, as the expression seems somewhat complex and giving a name to it, arguably, makes the code more readable.


Repository:
  rC Clang

https://reviews.llvm.org/D46000