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:
> sammccall wrote:
> > 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
> I don't like the idea of `targetDecl` becoming the swiss-army knife, it's
> complicated to use as is.
> The contract of `explicitReferenceTargets` is to expose only the decls
> written in the source code and nothing else, it's much simpler to explain and
> to use.
>
> It's useful for many things, essentially for anything that needs to answer
> the question of "what does this name in the source code refers to".
>
> The name could be clearer, the comments might need to be improved.
> The only crucial improvement that I'd attempt is getting rid of the `Mask`
> parameter, I think there was just one or two cases where it was useful.
>
>
> I don't like the idea of targetDecl becoming the swiss-army knife
Fair enough. I'll poke more in the direction of making it easier to compose
allTargetDecls then.
> The contract of explicitReferenceTargets is to expose only the decls written
> in the source code and nothing else, it's much simpler to explain and to use.
So I agree that the interface of targetDecl() is complicated, but I don't think
this one is simpler to explain or use - or at least it hasn't been
well-explained so far.
For example,
- "to expose only the decls written in the source code" - `vector<int>` isn't
a decl written in the source code.
- "find declarations explicitly referenced in the source code" - `return
[[{foo}]];` - the class/constructor isn't explicitly referenced, but this
function returns it.
- "what does this name in the source code refers to" - a template name refers
to the primary template, the (possible) partial specialization, and
specialization all at once. Features like find refs/highlight show the
equivalence class of names that refer to the same thing, but they don't prefer
the specialization for that.
- none of these explanations explains why the function is opinionated about
template vs expansion but not about alias vs underlying.
> 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 it's fine (and good!) that we this got added to unblock the hover work
and to understand more use cases for this API. But I do think it's time to fix
it now, and I'm not convinced a better name will do it (I can't think of a good
name to describe the current functionality, either). Let me put a patch
together and take a look?
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
