ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land.
lgtm ================ 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; ---------------- ilya-biryukov wrote: > 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. As discussed offline, two interfaces that do the same thing are a bit confusing. I would still suggest favoring minimal API and test-ability over simplified usage - two parameters aren't that better than one after all :) ================ Comment at: lib/AST/RawCommentList.cpp:352 + // comments::Lexer. Therefore, we just use default-constructed options. + CommentOptions DefOpts; + comments::CommandTraits EmptyTraits(Allocator, DefOpts); ---------------- ilya-biryukov wrote: > 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`. Makes sense. 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
