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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
