hokein added a comment.
> I totally understand if you want to make minimal changes to the code like
> this, but it would be great to simplify it a little bit without regressing
> too much, currently there is a knob for function locals and then another for
> parameters in declarations, then there is an implicit conditioning for
> function definitions.
> I would say it would be sensible to only have IndexFunctionLocals as an
> option and do full indexing of parameters, both in declarations and
> definitions if it is present (assuming this doesn't result in real
> regressions in the client code), but it is also OK to leave it as it is,
> since the mess is beyond the scope of this patch.
removing the `shouldIndexParametersInDeclarations` seems good to me, looks like
clangd is the only user of this flag. Regression is the concern -- as we are
changing the behavior of libindex, and the libindex doesn't seem to have enough
test coverage. I'd land this fix as it-is, and cleanup afterwards.
> But if you leave it as it is, I believe we'll still be missing some
> references to decls inside default arguments in clangd, for example:
>
> a.h:
>
> int var = 2;
> void foo(int x = var);
> a.cc
>
> void foo(int x) {
>
> va^r = x;
>
> }
> clangd won't have the reference occuring in default arg inside the index, as
> preambles are not indexed with IndexParametersInDeclarations option turned on.
Are you talking about dynamic index? clangd doesn't record any refs in preamble
by design -- as we skip function bodies in preamble
================
Comment at: clang/lib/Index/IndexDecl.cpp:102
auto *DC = Parm->getDeclContext();
+ IndexDefaultParmeterArgument(Parm, Parent);
if (auto *FD = dyn_cast<FunctionDecl>(DC)) {
----------------
kadircet wrote:
> this should move into the `else` clause down below, otherwise it would end up
> being indexed twice
My reading to the source is that
- `IndexCtx.handleDecl(Parm)` just reports the `Parm` decl occurrence, it
doesn't traversal the default arguments unfortunately :(
- the `if (const ParmVarDecl *Parm = dyn_cast<ParmVarDecl>(D))` here is only
triggered for ObjC cases, C++ never run into this code path :(
a conservative fix is that we only need the change on Line 117. I removed the
change in the current `if` clause.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74610/new/
https://reviews.llvm.org/D74610
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
