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

Reply via email to