[clang-tools-extra] r323149 - [clangd] Drop ~destructor completions - rarely helpful and work inconsistently

[Sam McCall via cfe-commits]

On Mon, Jan 29, 2018 at 5:31 PM, David Blaikie <dblaikie at gmail.com> wrote:

> Any chance of making this a really low priority completion?
>
(Just for clarity - this is a postfilter in clangd only, we haven't changed
the behavior of the Sema or libclang APIs)

We certainly have considered downranking such items, maybe it's the right
thing, but it's not without issues on the UX side:
 - It's far from the only candidate, other things that people want "at the
bottom" include inaccessible and unavailable members, injected type names,
operators... it gets crowded, so you still have to decide how to rank them.
 - LSP doesn't (currently) have any affordance to "grey out" results and
show users "this is the last good result, the ones below here are unlikely
to be useful". So the important end-of-list signal is lost.
There's also a bit of implementation complexity in having
second/third/fourth "tiers" of results - it adds invariants to the scoring
logic that make it harder to understand and modify.

(maybe just leaving in a FIXME or expected-fail check of some kind if it's
> not practical to implement it right now) For those handful of times when
> you actually want to do this.
>
re: practical to implement, there are a few problems with it, apart from
being rarely useful:
 - It completes after "foo.", but not "foo.~F", I guess because the parser
is in the wrong state
 - it completes ~basic_string, but not ~string

We do indirectly test that destructor completions are turned off in clangd.
I don't really know where to add the FIXMEs. I can add XFAIL tests to
c-index-test I think, is anyone likely to go looking for them?

On Mon, Jan 22, 2018 at 1:06 PM Sam McCall via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: sammccall
>> Date: Mon Jan 22 13:05:00 2018
>> New Revision: 323149
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=323149&view=rev
>> Log:
>> [clangd] Drop ~destructor completions - rarely helpful and work
>> inconsistently
>>
>> Modified:
>>     clang-tools-extra/trunk/clangd/CodeComplete.cpp
>>     clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
>>
>> Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
>> trunk/clangd/CodeComplete.cpp?rev=323149&r1=323148&r2=323149&view=diff
>> ============================================================
>> ==================
>> --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
>> +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Mon Jan 22 13:05:00
>> 2018
>> @@ -361,6 +361,10 @@ struct CompletionRecorder : public CodeC
>>            (Result.Availability == CXAvailability_NotAvailable ||
>>             Result.Availability == CXAvailability_NotAccessible))
>>          continue;
>> +      // Destructor completion is rarely useful, and works
>> inconsistently.
>> +      // (s.^ completes ~string, but s.~st^ is an error).
>> +      if (dyn_cast_or_null<CXXDestructorDecl>(Result.Declaration))
>> +        continue;
>>        Results.push_back(Result);
>>      }
>>    }
>>
>> Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
>> trunk/unittests/clangd/CodeCompleteTests.cpp?rev=
>> 323149&r1=323148&r2=323149&view=diff
>> ============================================================
>> ==================
>> --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
>> (original)
>> +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Mon
>> Jan 22 13:05:00 2018
>> @@ -461,7 +461,7 @@ TEST(CompletionTest, NoDuplicates) {
>>        {cls("Adapter")});
>>
>>    // Make sure there are no duplicate entries of 'Adapter'.
>> -  EXPECT_THAT(Results.items, ElementsAre(Named("Adapter"),
>> Named("~Adapter")));
>> +  EXPECT_THAT(Results.items, ElementsAre(Named("Adapter")));
>>  }
>>
>>  TEST(CompletionTest, ScopedNoIndex) {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180129/78fcf293/attachment.html>