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:
> > > 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?
> What's the proposed fix?
>
> I might miss some context with the examples you provided, but here's my view
> on how each of those should be handled:
> - `vector<int>`. This function **can** return decls, not written in the
> source code (e.g. template instantiations). References to those decls should
> be written in the source code explicitly, not the decls themselves.
> - `return {foo}`. I agree this one is a corner case that we should describe.
> It still provides sensible results that could be used by hover, etc.
> - "what does this name in the source code refers to". That's exactly what
> this function is about. We want to pick the most specific thing possible
> (i.e. the instantiation). Client code can go from instantiation to template
> pattern, it can't go the other way. There are exceptions where returning
> instantiation is not possible - instantiations of template type aliases,
> dependent template instantiations. We choose template pattern there, but we
> don't really loose any information (if we choose to return Decl, there's
> nothing else we can return).
> - "none of these explanations explains why the function is opinionated about
> template vs expansion but not about alias vs underlying." That I agree with,
> I think it's possible to make it opinionated there and remove the `Mask`
> parameter. I'm happy to take a look, but don't want to clash with you if
> you're doing the same thing. Would you want me to give it a try?
>
> What's the proposed fix?
(FWIW I wrote this post upside-down, because I don't/didn't have the ideas
worked out and this discussion has shaped my thinking a lot)
My proposal would be to evolve both `targetDecl()` and
`explicitReferenceTargets` into composable functions that help select the
desired results of `allTargetDecls()`. Policy would be encoded at callsites, so
functions would be quite specific (e.g. `preferTemplateInstantations`), or
data-driven `filter(DeclRelationSet)`.
Some reasoning below - basically I'm not convinced that the "do what I mean"
behavior is well-defined here.
> I agree this one is a corner case that we should describe. It still provides
> sensible results that could be used by hover, etc.
Yeah, it's the right behaviour. My point is that I don't think the policy being
implemented here is "explicitness" - another example from a recent patch, CTAD
`^vector x{1,2,3}` we want the instantiation.
Maybe it's "specificness", but this raises questions:
- how often is "most specific" the thing we want, vs "decl that's in source
code" or "everything for completeness"
- does the concept of 'specificness' generalize to alias vs underlying?
My impression is that it's *sometimes* what we want, but not overwhelmingly
often (e.g. not go-to-def or documentHighlights) and that it doesn't generalize
well[1]. If that's the case, I think this should be apolicy expressed at
callsites in features (`preferTemplateInstantiations` or something).
It would be nice if there's some global concept of "preferredness", but looking
at the needs of different features, I really don't see one - that's why
targetDecl is so complicated.
[1] Example of it not generalizing well - for hover we want instantiations over
patterns, but after discussing with Kadir I think we want *both* the alias and
the underlying so we can show the docs/association from the alias and the
structured info from the underlying.
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
