Page MenuHomePhabricator

[continued] Add a way to get the CXCursor that corresponds to a completion result
Needs RevisionPublic

Authored by svenbrauch on Sep 8 2017, 4:16 PM.

Details

Summary

See https://reviews.llvm.org/D13599, I cannot update that because it has a different author.

Added a very simplistic auto test for the use case that motivated addition of the feature.

Diff Detail

Event Timeline

svenbrauch created this revision.Sep 8 2017, 4:16 PM
milianw edited edge metadata.Sep 9 2017, 1:58 AM

lgtm, but could we get some more tests rather than only this one for templates?

test/CodeCompletion/templates.cpp
28

isn't the source location going to be printed everywhere now? are really no other test files affected by this?

I'll add some more, just wanted to post this to see if people think it's the right way to go.

Since the location is added at the end, all the other tests still pass, FileCheck just looks for the string and doesn't care about the extra suffix.

I added a few more tests and found two issues:

  • Cursors are also provided for implicit declarations. This is confusing and useless and shouldn't happen (e.g. for the implicit destructor, you don't want to get a cursor pointing to the class declaration). Instead, we return a null cursor now.
  • clang_sortCodeCompletionResults sorts the results vector, but leaves the cursors vector untouched. That destroys the proper association between result and cursor ... instead, we now store the cursor in the result.

Oh, one thing: I would like to remove the CursorKind member of the CXCompletionResult struct now since it is obsolete (same as clang_getCursorKind(Result->Cursor)). But, is that compatible with your API stability guarantees?

milianw requested changes to this revision.Sep 21 2017, 6:43 AM

I don't think this upholds the ABI guarantee, but maybe I'm wrong? Can someone else chime in please? @klimek ?

include/clang-c/Index.h
4738

can you add context to the patch please? i.e. re-upload via arc?

I think what you are doing here breaks the ABI though, and that was the reason for why I used a separate vector the last time, afaik.

This revision now requires changes to proceed.Sep 21 2017, 6:43 AM
svenbrauch edited edge metadata.

Sorry for the wait.

Any opinions on whether this compatibility promises? If it does, I can also fix Milian's variant with the extra list but it will be a bit slower.

milianw set the repository for this revision to rC Clang.Jan 9 2018, 4:47 AM
milianw requested changes to this revision.Jan 9 2018, 4:52 AM

I'm pretty sure this is not ABI compatible. You change the size of CXCompletionResult, so when anyone is iterating over the array in CXCompletionResults it will break. If you don't want to, I can try to find some time to take this over again ;-)

This revision now requires changes to proceed.Jan 9 2018, 4:52 AM

I know it breaks ABI compatiblity, I just don't know whether it needs to be kept (e.g. for clang-6.0). The alternative requires sorting by a separate index list which is a bit ugly (and slower). If you feel like working on it in the next few days that would of course be great, otherwise if it is clear that this solution doesn't work, I will pick it up again later. Thanks!