[PATCH] D74610: [clang][Index] Visit the default parameter arguements in libindex.

[Haojian Wu via Phabricator via cfe-commits]

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