[PATCH] D143112: [clang] Support parsing comments without ASTContext

[Tom Praschan via Phabricator via cfe-commits]

tom-anders marked an inline comment as done.
tom-anders added inline comments.


================
Comment at: clang/lib/AST/RawCommentList.cpp:227
+                      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID()),
+                      Allocator, SourceMgr.getDiagnostics(), Traits);
+}
----------------
kadircet wrote:
> tom-anders wrote:
> > I wasn't sure if it's worth making the `Diagnostics` parameter optional here - Our `SourceMgr` already has it anyway, does it impact performance significantly to use it, even though we don't need it (yet)?
> as things stand, Lexer, Sema and Parser expect a non-null diagnosticsengine so we have to pass something (i guess that's what you meant by making it optional).
> 
> the performance implications comes from two places:
> 1- extra analysis being performed in the face of malformed comments to produce "useful" diagnostics
> 2- creation cost of diagnostic itself, as all the data about ranges, decls etc. are copied around when constructing a diagnostic.
> 
> i don't know how costly the first action is today, it's unfortunately the case that would require a lot of code changes, if it's happening quite often in practice (especially when we're in codebases without proper doxygen comments)
> the latter is somewhat easier to address by providing our own "mock" diagnostics engine, that'll just drop all of these extra diagnostic data on the floor instead of copying around.
> 
> i think taking a look at the codebase for the first (searching for `Diag(...) <<` patterns in Comment{Lexer,Sema,Parser}.cpp should give some ideas) and assessing whether we're performing any extra complicated analysis would be a good start. i would expect us not to, and if that's the case i think we can leave it at that and don't try to make code changes.
> the latter we can address easily and we should.
1 - At first glance it indeed seems like there's no significant extra analysis performed, so I think we're good for now.
2 - I think this is already taken care of by `SourceManagerForFile` which just passes `nullptr` as the `DiagnosticConsumer`, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143112/new/

https://reviews.llvm.org/D143112