<div dir="ltr">Hi Sam,<div><br><div>I think it would be great if the current implementation was modularized in a way that would allow Clang and related tools to remove recursive tree visitors that are either abused for other purposes, or just duplicate work that some other visitors are doing, either partially or fully.</div><div> <br></div><div>I think starting out with a new implementation is not a bad approach. That said, I think it would be really good if the new implementation it was adopted by libIndex from the start, as this would allow:<br></div><div>- A clear path towards convergence of the new implementation and the old implementation </div><div>- More coverage for testing </div><div>The primary risk of not integrating this into libIndex directly is the high cost of transition to a unified implementation at a later stage by other adopters. Also, there's a real downside of not unifying code from the start (which we know too well unfortunately :( ): the general build and code size cost of additional tree visitor code in Clang and its tools. </div><div><br></div><div>I think while it's desirable to change implementation in an incremental way from the perspective of reviews and code history, sometimes it's not entirely practical as it's actually harder to see the full picture, and to qualify the change by doing additional testing by other adopters. I think for a change like this a patch that entirely rewrites a lot of the code in libIndex is acceptable. So my suggestion would be to:</div><div>- Work on unified implementation with libIndex from the start</div><div>- Migrate libIndex in one patch</div><div>- The reviewers or you might find ways to split up the patch into smaller, incremental changes during the review</div><div>- The reviewers will be able to test one patch only </div><div><br></div><div>Thanks,</div><div>Alex</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Aug 2019 at 10:26, Sam McCall <<a href="mailto:sammccall@google.com">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>TL;DR: I'd really like to extract the part that goes from AST node -> Decl + Role, but I don't know how to do it incrementally. Current plan would be to copy it to something parallel, and maybe try to port libIndex later. That sounds likely to end in permanent duplication though. Help!</div><div><br></div><div>--</div><div><br></div>Hi tooling folks,<div><br></div><div>There are several pieces in libIndex that are important for indexing, but also useful for other things. clangd (and I would guess libclang) has been (ab)using these, e.g. implementing "go to definition" by indexing the file and looking for decl references that overlap the cursor.</div><div>But performance, flexibility, and understanding tend to suffer, as the APIs don't always suit the problem space.</div><div><br></div><div>I think IndexingAction/IndexingContext do the following things:</div><div>A) expose macros while parsing/recorded in PP through a common interface</div><div>B) traverse the AST (according to some policy) to find AST nodes</div><div>C) determine which tokens in the source refer to those nodes</div><div>D) determine which decls AST nodes refer to, and with which roles</div><div>E) find decls related to an AST node (e.g. overrides)</div><div>There's also F) getSymbolInfo(), which is already split out.</div><div><br></div><div>These seem mostly[1] disjoint to me, and can each be replaced/dropped in some scenarios. examples:</div><div>- finding AST nodes (B) is sometimes better done with ASTSelection/SelectionTree/RecursiveASTVisitor.</div><div>- hover-to-evaluate-constant needs C but not D </div><div>- analyses like extract-variable care about D but not C or E</div><div><br></div><div>AST Node -> Decl is the largest and most useful chunk. I've built a standalone prototype which does pretty well on C++, and porting more of the logic from libIndex could bring it up to par.<br></div><div>The API is something like</div><div>  vector<pair<const Decl*, SymbolRoleSet>> findDecl(DynTypedNode,</div><div>     ReferencePolicy, // lexical references only, or semantic ones like `decltype(...)` too?</div><div>     AliasPolicy, // for typedefs etc, return the alias, the underlying decl, or both?</div><div>     TemplatePolicy, // return specialization, the relevant pattern, the primary template, or some combination?</div><div>  )</div><div>The Policy options allow us to more directly express features and fix some bugs.</div><div><br></div><div>My main concern is that I'm not sure how to eventually (ideally incrementally!) use this in libIndex.</div><div>The natural implementation is a set of mutually recursive functions to handle Decl/Type/Stmt etc (much like libIndex's structure), so before calling any of them, all must be complete and correct.</div><div>And SymbolRelations seem to sometimes need information from the AST traversal, and sometimes information from the node->decl walk, so it's hard to see how they can be separated.</div><div><br></div><div>So though it seems insane to have two copies of this logic, I find myself leaning toward a separate implementation. Is there a better way out than this? How much value is there in putting it in Tooling vs locally in clangd?</div><div><br></div><div>Cheers, Sam</div><div><br></div><div>[1] B and E seem fairly tangled, actually.</div></div>
</blockquote></div>