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