kadircet marked an inline comment as done.
kadircet added inline comments.
================
Comment at: clang/lib/Index/IndexingAction.cpp:142
+ ShouldSkipFunctionBody =
+ [ShouldTraverseDecl(Opts.ShouldTraverseDecl)](const Decl *D) {
+ return !ShouldTraverseDecl(D);
----------------
sammccall wrote:
> kadircet wrote:
> > are we happy with the copy here ?
> >
> > I am a little uneasy, as these functions can preserve state. Currently the
> > one used in here and the one used in `IndexingContext::indexTopLevelDecl`
> > are different copies.
> >
> > Maybe rather progate a null function into the `IndexASTConsumer` and create
> > this default lambda in there, making use of the function inside
> > `IndexingContext::IndexOpts` without a copy?
> > are we happy with the copy here ?
>
> Note that we already have a copy: we take IndexingOptions by const ref and
> createIndexingASTConsumer copies it, including the function. So anything that
> relies on the identity of the function passed in, or breaks semantics of
> copying, is busted already.
>
> as for internal state (e.g. a cache)... I'd agree if this were a heavily used
> library function (find a way to capture by pointer/shared_ptr). But it's not,
> and neither of the in-tree callers work this way - I'm not sure it's worth
> spending API complexity on.
>
> > Maybe rather progate...
>
> Hmm, I like the idea that these are clearly separate options at the lower
> internal layer, and that blurs the lines a bit.
> (I'd like *more* making them completely the same, but halfway isn't a happy
> place to be I think)
> as for internal state (e.g. a cache)... I'd agree if this were a heavily used
> library function (find a way to capture by pointer/shared_ptr). But it's not,
> and neither of the in-tree callers work this way - I'm not sure it's worth
> spending API complexity on.
SG. I wasn't concerned about the copying but rather lambda invoked in
`ShouldSkipFunctionBody` and `ShouldTraverseDecl` being different. I agree that
preserving internal state is not that important for current in-tree uses, just
wanted to make sure the discrepancy didn't go unnoticed :D
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80296/new/
https://reviews.llvm.org/D80296
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
