[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:
> > 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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71596