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

[Sam McCall via clangd-dev]

I agree with the thrust of your tests - that we should rarely be providing
these completions.

(As noted on the patch, the tests themselves could probably be combined
with the existing related tests, rather than adding lots of new ones).

Cheers, Sam

On Wed, Sep 26, 2018 at 5:14 PM Jan Korous <jkorous at apple.com> wrote:

> I totally missed that in my examples all those special methods were
> declared only implicitly. Thanks Ilya for pointing that out!
>
> For the record - it seems like we are currently providing code completion
> for out-of-class method definitions.
> struct foo { void foomethod(); } foo::^
> // returns among others also foomethod
>
> If you think that providing struct name, ctor, dtor and assignment
> completion for this case without degrading quality of results or adding
> noise somewhere else is too big hassle right now I don’t mind removing
> these.
>
> I think the best way forward is to agree on some testcases which would
> specify our requirements against which I could write my patches. (Real TDD
> first time in my life, yay!)
>
> Could you please take a look at these tests to check we are on the same
> page?
> https://reviews.llvm.org/D52554
>
> I will look into the issue with “foo::~^" separately. I guess Sam might be
> right about a parser bug as I remember seeing some funny-looking callstack
> in clang under lldb.
>
> Cheers,
>
> Jan
>
> On 25 Sep 2018, at 19:27, Sam McCall <sammccall at google.com> wrote:
>
> 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/20181002/e811b8d6/attachment.html>