[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan abandoned this revision.
zyounan added a comment.

Closing this in favor of D156605 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan planned changes to this revision.
zyounan added a comment.

In D155370#4552967 , @nridge wrote:

> I was thinking of functions with different names (with a common prefix) and 
> different signatures, where the signature could be a useful piece of 
> information in addition to the name to help you choose the right one.

Ah, that makes sense! Honestly, I've never thought the signature could assist 
such a scenario.

> Note, we have a flag that affects this, `--completion-style=detailed`.

Thanks. Just learned this flag.

Apart from the `Signature`, I still have two improvements for `CanBeCall`. I 
think it's better to close this review and merge these two changes into one 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D155370#4550042 , @zyounan wrote:

> in the context where `CanBeCall=false`, parameters don't disambiguate against 
> the overloads, so we'd end up with the same function name after selecting the 
> candidate

I was thinking of functions with different names (with a common prefix) and 
different signatures, where the signature could be a useful piece of 
information in addition to the name to help you choose the right one.

For example, consider:

  struct Thread {
void sleep_until(time_point);
void sleep_for(time_duration);
  };
  
  ::sleep^

Maybe the distinction between the functions is less obvious from their names 
(`until` vs. `for`), but more obvious from the type of argument they take.

In D155370#4550042 , @zyounan wrote:

> An explicit cast may be required to perform overload resolution if necessary 
> , but that 
> should occur before the completion point `::Prefix^`.

(That would be an interesting future enhancement to consider for the overload 
set case.)

> OTOH, we don't present parameters for overloads in the candidates (using the 
> clangd without this patch):
>
> F28542806: image.png 
>
> (At least for VSCode, I'm not sure if others behave the same.)

Note, we have a flag that affects this, `--completion-style=detailed`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

In D155370#4545763 , @nridge wrote:

> (e.g. maybe you're looking for one with a particular parameter type).

I understand the second point that it'd be nice to offer the user a chance to 
see the arguments in order to help decide if the function is appropriate -- 
although in the context where `CanBeCall=false`, arguments don't disambiguate 
against the overloads, so we'd end up with the same function name after 
selecting the candidate. (An explicit cast may be required to perform overload 
resolution if necessary 
, but that 
should occur before the completion point `::Prefix^`.)

OTOH, we don't present arguments for overloads in the candidates:

F28542806: image.png 

(At least for VSCode, I'm not sure if others behave the same.)

So, I don't think it is that important to retain the `Signature`. Any thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I'm not sure how I feel about dropping the parameters from the signature in the 
`CanBeCall = false` case.

I can see arguments in both directions:

- On the one hand, dropping the parameters makes the text that is inserted more 
consistent with the text that is shown for the proposal.
- On the other hand, imagine you're typing in a `CanBeCall = false` context 
(e.g. `::Prefix^`), and there are several matching method names that 
begin with `Prefix` that you are trying to choose from. Seeing their signatures 
might be a useful piece of additional context to help you choose the right one 
(e.g. maybe you're looking for one with a particular parameter type).

On the whole, I think I lean towards the second point being more important, and 
therefore towards keeping the signatures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1407
 
   R.FunctionCanBeCall =
   CurrentClassScope &&

zyounan wrote:
> The current heuristic results in the following:
> 
> ```
> struct Base {
>   void foo(int);
> };
> struct Derived : Base {
>   void foo(float);
> };
> 
> int main() {
>   Derived d;
>   d.Base::^// FunctionCanBeCall for `foo` is false.
> }
> ```
> 
> In situations where it is intended to invoke base member functions hidden by 
> subclasses, we shouldn't remove parentheses, IMO.
D156605 to address this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1407
 
   R.FunctionCanBeCall =
   CurrentClassScope &&

The current heuristic results in the following:

```
struct Base {
  void foo(int);
};
struct Derived : Base {
  void foo(float);
};

int main() {
  Derived d;
  d.Base::^// FunctionCanBeCall for `foo` is false.
}
```

In situations where it is intended to invoke base member functions hidden by 
subclasses, we shouldn't remove parentheses, IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155370: [CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thanks for the insightful suggestions!
(Apologies for my late update. Just had a really busy week.)

As suggested, I left the CCR intact and handled these functions in 
`CodeCompletionResult::createCodeCompletionStringForDecl`. I think this 
preserves the Declaration, right? (While I think we //could// get the 
associated Decl if using `RK_Pattern`, however the current approach looks more 
terse to me.)

I also noticed that the previous implementation did not consider function 
templates. For example,

  struct S {
template 
void foo(T);
  
void bar();
  };
  
  ::bar^ // getting `S::bar`
  ::foo^ // getting `S::foo(T)`!

This brings a discrepancy, which I have also fixed in this patch. I hope this 
doesn't cause too much inconvenience for the review :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits