nridge requested changes to this revision.
nridge added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1297
+ FunctionCanBeCall =
+ MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase);
+ }
----------------
nit: we can avoid the computation in this block if `FunctionCanBeCall` was
already initialized to `true` above
================
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)
{
----------------
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 */)`.
================
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:
> > 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>`?
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
