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

[Eric Liu via Phabricator via cfe-commits]

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