ilya-biryukov added inline comments.
================
Comment at: include/clang/AST/RawCommentList.h:138
+ /// the overload with ASTContext in the rest of the code.
+ std::string getFormattedText(const SourceManager &SourceMgr,
+ DiagnosticsEngine &Diags) const;
----------------
ioeric wrote:
> I think we can get rid of the interface that takes `ASTContext`? If
> `SourceManager` and `Diags` are sufficient, I don't see why we would want
> another interface for ASTContext.
Two reasons that come to mind: it's simpler to use and follows the API of
`getBriefText`. If not for mocking the tests, I would totally only keep the
`ASTContext`-based one, since it does not really make any sense to create
`RawComment` without `ASTContext` for any reason other than testing.
================
Comment at: lib/AST/RawCommentList.cpp:352
+ // comments::Lexer. Therefore, we just use default-constructed options.
+ CommentOptions DefOpts;
+ comments::CommandTraits EmptyTraits(Allocator, DefOpts);
----------------
ioeric wrote:
> I'm not quite sure about this. Could we just require a `CommandTraits` in the
> interface? And only make this assumption in tests?
I think we shouldn't add this to params, the whole point of this function is to
do parsing that ignores the commands and the `CommandTraits`. The fact that
lexer still needs them is because we haven't extracted a simpler interface from
`Lexer` that doesn't rely on unused params, i.e. `CommandTraits` and
`Allocator`.
================
Comment at: unittests/AST/CommentTextTest.cpp:32
+ std::string formatComment(llvm::StringRef CommentText) {
+ llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> EmptyFS(
+ new vfs::InMemoryFileSystem);
----------------
ioeric wrote:
> `SourceManagerForFile` added in D46176 should save you a few lines here. (I'm
> landing it right now...)
Thanks!
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
