nridge added a comment. Ok, I've made `HeuristicResolver` nullable, in the sense that I've added null checks arounds its use in `TargetFinder`.
I haven't refactored the tests to pass in a null resolver / annotate the ones that do or don't need one. Would you like that done in this patch, or a follow-up? ================ Comment at: clang-tools-extra/clangd/HeuristicResolver.h:60 + // or more declarations that it likely references. + std::vector<const NamedDecl *> resolveExprToDecls(const Expr *E); + ---------------- sammccall wrote: > nridge wrote: > > sammccall wrote: > > > This is pretty vaguely defined. From reading the implementation, this can > > > choose to return two things: > > > > > > 1. a ValueDecl representing the value of E > > > 2. a TypeDecl representing the type of E > > > > > > Case 1 is used by heuristic go-to-def, which never hits case 2 because it > > > only passes certain kinds of E. > > > Case 1+2 are used internally by the heuristic resolver to resolve base > > > types, and are unified by resolveDeclsToType which handles them as two > > > cases. > > > > > > This doesn't seem like a clean public API :-) It also seems to lead to > > > some convolution internally. > > > > > > --- > > > > > > I think we should distribute the work of this function into a few smaller > > > functions with couple of flavors. > > > > > > Some that deal with specific node types, extracted from > > > resolveExprToDecls: > > > - `Decl* memberExprTarget(CXXDependentMemberExpr*)`. Uses exprToType > > > (for the base type), pointee (for arrow exprs), resolveDependentMember > > > (the final lookup). > > > - `Type* callExprType(CallExpr*)`. Uses exprToType (for the callee type). > > > - `Decl* declRefExprTarget(DependentScopeDeclRefExpr*)` - this is > > > currently a trivial wrapper for resolveDependentMember. > > > > > > And some that are more generic building blocks: > > > - `Type* exprType(Expr*)`, which tries to handle any expr, dispatching > > > to the node-specific function and falling back to Expr::getType(). > > > - `getPointeeType(Type*)` and `resolveDependentMember(...)` as now > > > > > > It's not entirely clear what should be public vs private, but one idea I > > > quite like is: all the node-specific methods are public, and all the > > > building blocks are private. > > > - This would mean adding some more node-specific methods to cover the > > > external callers of `resolveDependentMember`, but in each case I looked > > > at this seems to make sense. > > > (This neatly deals with the slightly grungy signature of > > > resolveDependentMember by hiding it) > > > - it also mostly answers the question about contracts on non-dependent > > > code: most non-dependent node types simply can't be passed in > > Thanks, you make some good points about this method being messy (and > > particularly that we're duplicating some "resolve the type of a > > non-dependent expression" logic, which has bothered me as well). > > > > I like your proposed breakdown into smaller methods, and I partially > > implemented it. > > > > A couple of notes: > > > > * I had to keep the [MemberExpr > > case](https://searchfox.org/llvm/rev/b3d7e761e347d562333893652dcf3837fa55d777/clang-tools-extra/clangd/FindTarget.cpp#218) > > around. It could not be subsumed by `Expr::getType()`, because for a > > `MemberExpr` that returns [this > > thing](https://searchfox.org/llvm/rev/b3d7e761e347d562333893652dcf3837fa55d777/clang/include/clang/AST/BuiltinTypes.def#283), > > while `getMemberDecl()->getType()` returns the function type of the member > > function, and we want the latter (for [this call > > site](https://searchfox.org/llvm/rev/b3d7e761e347d562333893652dcf3837fa55d777/clang-tools-extra/clangd/FindTarget.cpp#205)). > > * We don't quite achieve "non-dependent node types simply can't be passed > > in", because `resolveCallExpr()` can be called with a non-dependent call > > expr. However, it currently has no external callers -- perhaps we should > > make it private? > > > > I left `resolveDependentMember()` public (and left its external callers > > alone) for now. I can make it private and add additional public wrappers if > > you'd like, but I'm wondering what the motivation is (especially as you've > > described it as "a really nice example of a heuristic lookup with a clear > > contract" :-)). > Those notes/caveats seem totally fine to me. > > > resolveCallExpr ... perhaps we should make it private? > > left resolveDependentMember() public ... I can make it private... I'm > > wondering what the motivation is > > I just think it's a little easier to understand how to use this class, and > how to maintain/extend it, if the public methods form a consistent set with > the same level of abstraction. Having all the public methods handle node > types is tempting from this point of view. > > I won't insist on this. We can inline the handling of UnusedUsingValueDecl > into the caller as it happens to be simple, but I don't see a strong reason > to do so. Your argument for consistency, together with the fact that this allows moving the `Filter` lambdas out of the header file, has convinced me. Done! ================ Comment at: clang-tools-extra/clangd/ParsedAST.h:113 + HeuristicResolver *getHeuristicResolver() const { return Resolver.get(); } + ---------------- sammccall wrote: > is this intended to be nullable? (If not, why the pointer type?) I changed this to return `const HeuristicResolver *`, but kept it as a pointer for a couple of reasons: * I couldn't make `ParsedAST` store `HeuristicResolver` by value because it would make `ParsedAST` non-movable (`HeuristicResolver` is non-movable because it has a member of reference type (`ASTContext&`)). * The consumer functions (targetDecl() etc.) expect a pointer, so this avoids writing `&AST->getHeuristicResolver()` at every call site). ================ Comment at: clang-tools-extra/clangd/ParsedAST.h:150 CanonicalIncludes CanonIncludes; + std::unique_ptr<HeuristicResolver> Resolver; }; ---------------- nridge wrote: > sammccall wrote: > > Optional<HeuristicResolver> or simply HeuristicResolver? > I wanted to make it just `HeuristicResolver` but it's accessed through a > `const ParsedAST` which would only expose a `const HeuristicResolver *`, and > I had already sprinkled `HeuristicResolver *` (without the const) everywhere > :) > > I'll clean this up after we settle the nullability question. See previous comment for why I couldn't make it `HeuristicResolver`. (`Optional` doesn't work for the same reason.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
