<div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div>Thanks for looking into this!</div><div><br></div><div dir="ltr">On Tue, Sep 25, 2018 at 5:17 PM Jan Korous via clangd-dev <<a href="mailto:clangd-dev@lists.llvm.org">clangd-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div>2. We currently skip constructors but include destructors, assignment operators and struct/class name after double colon.<br></div><div><br></div><div>struct foo {}; foo::^</div><div>// produces 4 results - struct foo itself, copy assignment, move assignment, destructor</div></div></blockquote><div>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.</div><div><br></div><div>Possible exceptions:</div><div>  struct bar : foo { using foo::^foo }</div><div>(probably looks the same to the parser)</div><div>This seems rare enough though.</div><div><br></div><div>Bar& Bar::^operator=(const Bar&) { }</div><div>(completion of out-of-line implementation)</div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div>- 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.</div></div></div></blockquote><div>I'm not sure what you're suggesting we complete here.</div><div>If it's literally "foo::foo" then that's the injected class name, struct or class seems correct.</div><div>If it's a whole constructor "foo::foo(someargs)", with one for each overload, that seems rarely correct/useful.</div><div>  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div>- 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).</div></div></div></blockquote><div>I think we talked past each other then :-)</div><div>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: <a href="https://godbolt.org/z/L4US-Q">https://godbolt.org/z/L4US-Q</a></div><div><br></div><div><div>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... </div><br class="gmail-Apple-interchange-newline"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div>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".</div></div></div></blockquote><div>Are there common patterns when these are actually useful completions?</div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div>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.</div><div><br></div><div>struct foo {}; foo::~^</div><div>// produces 66 results - including keywords and probably built-in macros and struct/class name but not constructor or destructor</div><div><br></div><div>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).</div></div></blockquote><div>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!</div></div></div></div>