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

[Jan Korous via clangd-dev]

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 <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 <mailto: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 <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/20180926/3b70b525/attachment.html>