This is an archive of the discontinued LLVM Phabricator instance.

[libclang] Expose completion result kind in `CXCompletionResult`
AcceptedPublic

Authored by egorzhdan on Oct 27 2022, 6:46 AM.

Details

Summary

This allows clients of libclang to check whether a completion result is a keyword. Previously, keywords had CursorKind == CXCursor_NotImplemented and it wasn't trivial to distinguish a keyword from a pattern.

This change moves CodeCompletionResult::ResultKind to clang-c under a new name CXCompletionResultKind. It also tweaks c-index-test to print the result kind instead of NotImplemented, and adjusts the tests for the new output.

rdar://91852088

Diff Detail

Event Timeline

egorzhdan created this revision.Oct 27 2022, 6:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 6:46 AM
Herald added a subscriber: kadircet. · View Herald Transcript
egorzhdan requested review of this revision.Oct 27 2022, 6:46 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 27 2022, 6:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet added inline comments.Oct 27 2022, 6:56 AM
clang/include/clang/Sema/CodeCompleteConsumer.h
755 ↗(On Diff #471154)

i don't follow the reason for replacing this struct with CXCompletionResultKind and renaming occurrences in half of the codebase. can we keep this around, introduce the new enum type into Index.h and have clang/tools/libclang/CIndexCodeCompletion.cpp do the conversion from Sema type to Index type instead?

Unless I am missing some other requirement for doing so. i feel like the conceptual dependency from Sema to Index is one we should get rid of, rather than make tighter.

egorzhdan updated this revision to Diff 471838.Oct 30 2022, 8:27 AM

Fix clangd compilation

Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2022, 8:27 AM
egorzhdan added inline comments.Oct 30 2022, 8:28 AM
clang/include/clang/Sema/CodeCompleteConsumer.h
755 ↗(On Diff #471154)

This was mostly done to be consistent with the way CXCursorKind and CXAvailabilityKind are declared in Index.h and used as fields of CodeCompletionResult. I think updating the enum name in a few source files shouldn't be a major problem, but I can avoid it if you feel strongly against this approach.

kadircet added inline comments.Oct 31 2022, 2:24 AM
clang/include/clang/Sema/CodeCompleteConsumer.h
755 ↗(On Diff #471154)

I completely agree with the consistency argument, but IMO the conceptual dependency from clang-sema to clang-cindex is just wrong.

cindex is mostly implemented by libclang, which is part of clang-tools. all of this works today because clang-sema is only using declarations visible in headers and doesn't properly depend on libclang in the build files.

Hence I believe rather than preserving consistency here, we should be moving towards breaking that dependency over time. Therefore I was opposing the idea of introducing another type usage that'll make this dependency more strong.

egorzhdan added inline comments.Oct 31 2022, 11:18 AM
clang/include/clang/Sema/CodeCompleteConsumer.h
755 ↗(On Diff #471154)

all of this works today because clang-sema is only using declarations visible in headers and doesn't properly depend on libclang in the build files.

Oh I see, this is a great point, I didn't realize that. I'll update the patch.

egorzhdan updated this revision to Diff 472171.Oct 31 2022, 3:59 PM

Preserve CodeCompletionResult::ResultKind and do the conversion to
CXCompletionResultKind in CIndexCodeCompletion.cpp.

bnbarham accepted this revision.Oct 31 2022, 4:19 PM

Thanks Egor!

This revision is now accepted and ready to land.Oct 31 2022, 4:19 PM
arphaman accepted this revision.Nov 10 2022, 3:34 PM

LGTM!

omjavaid reopened this revision.Nov 14 2022, 12:30 AM
omjavaid added a subscriber: omjavaid.

Hi this broke https://lab.llvm.org/buildbot/#/builders/245/builds/761
I have reverted the change to make the buildbot green.

This revision is now accepted and ready to land.Nov 14 2022, 12:30 AM