sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/FindTarget.h:194
+llvm::SmallVector<const NamedDecl *, 1>
+explicitReferenceTargets(ast_type_traits::DynTypedNode N,
+                         DeclRelationSet Mask = {});
----------------
ilya-biryukov wrote:
> No need to fix this.
> 
> The name could probably be better, but we can fix this later. Don't have any 
> good ideas.
I think as a public API this should be clearer on how/when to use and its 
relation to other things (planning to send a patch, but wanted to discuss a bit 
here first).

- This is essentially a filter of allTargetDecls (as is targetDecl), but the 
relationship between the two isn't clear. They should have closely related 
names (variant) or maybe better be more orthogonal and composed at the callsite.
- The most distinctive word in the name is `explicit`, but this function 
doesn't actually have anything to do with explicit vs non-explicit references 
(just history?)
- It's not clear to me whether we actually want to encapsulate/hide the 
preference for instantiations over templates here, or whether it should be 
expressed at the callsite because the policy is decided feature-by-feature. 
`chooseBest(...)` vs `prefer(TemplateInstantiation, TemplatePattern, ...)`. The 
latter seems safer to me, based on experience with targetDecl so far.

A couple of ideas:
 - caller invokes allTargetDecls() and then this is a helper function 
`prefer(DeclRelation, DeclRelation, Results)` that mutates Results
 - targetDecl() becomes the swiss-army filter and accepts a list of 
"prefer-X-over-Y", which it applies before returning the results with flags 
dropped


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71596/new/

https://reviews.llvm.org/D71596



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to