[clangd-dev] RFC: Constructor/destructor code completion

[Sam McCall via clangd-dev]

Thanks for looking into this!

On Tue, Sep 25, 2018 at 5:17 PM Jan Korous via clangd-dev <
clangd-dev at lists.llvm.org> wrote:

> 2. We currently skip constructors but include destructors, assignment
> operators and struct/class name after double colon.
>
> struct foo {}; foo::^
> // produces 4 results - struct foo itself, copy assignment, move
> assignment, destructor
>
I think there's a fair case to be made we shouldn't include any of these.
Explicitly referring to constructor, destructor, operators, injected class
name are all rare enough that these may add more noise than value. I'm
personally not opposed to removing some completions that are technically
correct but rarely useful.

Possible exceptions:
  struct bar : foo { using foo::^foo }
(probably looks the same to the parser)
This seems rare enough though.

Bar& Bar::^operator=(const Bar&) { }
(completion of out-of-line implementation)
I'm not sure how these completions currently work, but I'm sure we can make
sure these only trigger where an out-of-line definition is actually
plausible.

- It seems to me that by including struct/class name we effectively include
> constructor code completion which makes sense in this situation but we
> shouldn’t put it’s kind as struct/class.
>
I'm not sure what you're suggesting we complete here.
If it's literally "foo::foo" then that's the injected class name, struct or
class seems correct.
If it's a whole constructor "foo::foo(someargs)", with one for each
overload, that seems rarely correct/useful.


> - What seems odd to me in this context is that we use void as return type
> of destructor (and had we not skipped constructor, it’s return type might
> be unexpected too). We previously discussed destructor return type with Sam
> in [1] and I agreed that it’s fine under false impression that we aren’t
> displaying destructors anyway (off-by-one error in my test case).
>
I think we talked past each other then :-)
I'm not sure what you can do with a destructor other than declare it or
call it, you can't take its address. So presumably the expression we're
completing here is a destructor *call*: ~Foo(). The type of that *is* void:
https://godbolt.org/z/L4US-Q

We can omit return type but I'm a bit uncomfortable with the approach of
omitting it in the CodeCompletionResult for aesthetic reasons, because
return type matching the context is a completion quality signal. Maybe in
this particular case we get away with it, but...

I suggest that we don’t include struct/class name but include both
> constructor (without any return type == no “detail” in LSP) and destructor
> (without any return type == no “detail” in LSP) when code completion
> context is "struct/class name followed by double colon".
>
Are there common patterns when these are actually useful completions?
My intuition is that these are rarely useful, and so attempting to handle
them correctly may come out worse overall by adding complexity or
false-positive results and getting little true-positives in return. Just a
guess though.


> 3. We currently skip both constructors and destructors but include
> struct/class name after double colon and tilde but also include lot of
> other results.
>
> struct foo {}; foo::~^
> // produces 66 results - including keywords and probably built-in macros
> and struct/class name but not constructor or destructor
>
> This seems wrong to me. I suggest we skip unrelated keywords and
> struct/class name but we should include destructor (with proper insertText
> to not repeat the leading tilde).
>
I think this is just a parser bug that nobody's bothered to fix. The parser
is designed to parse valid C++, code completion is bolted on. It probably
doesn't recover from the syntax error that is foo::~f in a useful way.
Please fix it if you can!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/clangd-dev/attachments/20180925/42d035b5/attachment-0001.html>