hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/HeuristicResolver.cpp:33 +const Type *HeuristicResolver::resolveDeclsToType( + const std::vector<const NamedDecl *> &Decls) const { ---------------- the reason to promote this utility function to a class method seems to be able to access the member field ASTContext. If so, I'd probably just passing the ASTContext as a parameter rather then making them member methods. ================ Comment at: clang-tools-extra/clangd/HeuristicResolver.cpp:38 + if (const auto *TD = dyn_cast<TypeDecl>(Decls[0])) { + return Ctx.getTypeDeclType(TD).getTypePtr(); + } ---------------- nridge wrote: > I wanted to call out the change on this line which is easy to overlook due to > the code move: > > We were previously calling > [TypeDecl::getTypeForDecl()](https://searchfox.org/llvm/rev/5b657f50b8e8dc5836fb80e566ca7569fd04c26f/clang/include/clang/AST/Decl.h#3301), > but that's actually a low-level accessor for a cached value. The function > that also populates the value if needed is `ASTContext::getTypeDeclType()`, > hence I switch to using that. thanks for the highlight, I think this change makes sense. ================ Comment at: clang-tools-extra/clangd/HeuristicResolver.cpp:56 + if (const auto *DNT = T->getAs<DependentNameType>()) { + T = resolveDeclsToType(resolveDependentNameType(DNT)); + if (!T) ---------------- Is the `resolveDeclsToType` call necessary here? The code path we're doing is `dependent-type => Decl* => Type => Decl*`, the later two steps seems redundant, can we just use the Decl result from `resolveDependentNameType()`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152645/new/ https://reviews.llvm.org/D152645 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits