[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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 = {});
----------------
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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71596