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

[David Blaikie via cfe-commits]

Fair - maybe just a comment in the test case (and/or the source code that
handles this) saying "hey, let's make sure dtors aren't here - but if/when
there's support for a low priority completion group, these should appear
there" or the like.

On Mon, Jan 29, 2018 at 9:49 AM Sam McCall <sam.mccall at gmail.com> wrote:

> 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/f2a003c9/attachment-0001.html>