zyounan added a comment.
Thank you Nathan for your constructive opinions! I've updated this patch again,
hopefully this becomes better.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
- // When completing a non-static member function (and not via
- // dot/arrow member access) and we're not inside that class' scope,
- // it can't be a call.
+ // Decide whether or not a non-static member function can be a call.
if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol)
{
----------------
nridge wrote:
> zyounan wrote:
> > sammccall wrote:
> > > This is confusing: the `CCC_Symbol` test is part of the specific
> > > heuristics being used (it's the "not via dot/arrow member access" part,
> > > right?) but you've moved the comment explaining what it does.
> > >
> > > Also this function is just getting too long, and we're inlining more
> > > complicated control flow here.
> > > Can we extract a function?
> > >
> > > ```
> > > const auto *Method = ...;
> > > if (Method & !Method->isStatic()) {
> > > R.FunctionCanBeCall = canMethodBeCalled(...);
> > > }
> > > ```
> > > it's the "not via dot/arrow member access" part
> >
> > (Sorry for being unaware of the historical context). But I think
> > `CCC_Symbol` should mean "we're completing a name such as a function or
> > type name" per its comment. The heuristic for dot/arrow member access
> > actually lies on the next line, i.e., if the completing decl is a
> > CXXMethodDecl.
> >
> > > Can we extract a function?
> >
> > Sure.
> The check for `CompletionContext.getKind()` is in fact a part of the
> heuristic:
>
> * for `f.method`, the kind is `CCC_DotMemberAccess`
> * for `f->method`, the kind is `CCC_ArrowMemberAccess`
> * for `f.Foo::method` and `f->Foo::method`, the kind is `CCC_Symbol`
>
> (I realize that's a bit inconsistent. Maybe the `f.Foo::` and `f->Foo::`
> cases should be using `DotMemberAccess` and `ArrowMemberAccess` as well?
> Anyways, that's a pre-existing issue.)
>
> So, the `if (CompletionContext.getKind() ==
> clang::CodeCompletionContext::CCC_Symbol)` check is what currently makes sure
> that in the `f.method` and `f->method` cases, we just keep `FunctionCanBeCall
> = true` without having to check any context or expression type.
>
> I think it may be clearest to move this entire `if` block into the new
> function (whose name can be generalized to `canBeCall` or similar), and here
> just unconditionally set `R.FunctionCanBeCall = canBeCall(CompletionContext,
> /* other things */)`.
> I think it may be clearest to move this entire if block into the new function
Nice suggestion. I'll take it.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
// containing all of the arguments up to the first deducible argument.
+ // Or, if this isn't a call, emit all the template arguments
+ // to disambiguate the (potential) overloads.
----------------
zyounan wrote:
> nridge wrote:
> > zyounan wrote:
> > > nridge wrote:
> > > > 1. If this is not a call, we can avoid running the
> > > > `Sema::MarkDeducedTemplateParameters` operation altogether.
> > > >
> > > > 2. A future improvement we could consider: if this is not a call, try
> > > > to detect cases where the template parameters can be deduced from the
> > > > surrounding context (["Deducing template arguments taking the address
> > > > of a function template
> > > > "](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe add a FIXME
> > > > for this?
> > > > avoid running the Sema::MarkDeducedTemplateParameters operation
> > > > altogether
> > >
> > > I think doing so could cause too many adjustments to the flow, and I'm
> > > afraid that the `Sema::MarkDeducedTemplateParameters` would be required
> > > again when we decide to implement point 2.
> > >
> > > I'm adding a FIXME anyway but leave the first intact. However, I'd be
> > > happy to rearrange the logic flow if you insist.
> > I realized there's actually an open question here: if `FunctionCanBeCall ==
> > false`, do we want to include **all** the template parameters, or just the
> > non-deducible ones?
> >
> > Let's consider an example:
> >
> > ```
> > class Foo {
> > template <typename T, typename U>
> > T convertTo(U from);
> > };
> >
> > void bar() {
> > Foo::^
> > }
> > ```
> >
> > Here, `U` can be deduced but `T` cannot.
> >
> > The current behaviour is to complete `convertTo<T>`. That seems appropriate
> > for a **call**, since `U` will be deduced from the call. But since this is
> > not a call, wouldn't it be better to complete `convertTo<T, U>`?
> > But since this is not a call, wouldn't it be better to complete
> > convertTo<T, U>?
>
> (Ugh, this is exactly this patch's initial intention, but how...?)
>
> Oh, I made a mistake here: I presumed `LastDeducibleArgument` would always be
> 0 if we're in a non-callable context, but this is wrong! This variable is
> calculated based on the function parameters, which ought to be equal to the
> number of template parameters deducible in function parameters.
>
> ( I used to duplicate this `if` block for the `!FunctionCanBeCall` case but
> finally, I merged these two erroneously. :-( )
> A future improvement we could consider: if this is not a call, try to detect
> cases where the template parameters can be deduced from the surrounding
> context ("Deducing template arguments taking the address of a function
> template "). Maybe add a FIXME for this?
UPD: I've looked into the deduction implementation and found it not difficult
to add such a heuristic: We have `PreferredType` that describes the type in a
declaration, e.g., `void (*)(int, int)` and
`DeduceTemplateArgumentsByTypeMatch` in Sema to tell if the template parameter
type `P` could be deduced from the 'synthesized' type `A` (That is, the
declaring type `void (int, int)`, for example).
I suppose we could apply such a deduction for each candidate and eliminate its
template parameters in the result if the deduction succeeds - But let me leave
that for the next patch(es) so I don't overload you. :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156605/new/
https://reviews.llvm.org/D156605
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
