[PATCH] D136844: [libclang] Expose completion result kind in `CXCompletionResult`

Mon Oct 31 02:24:02 PDT 2022

kadircet added inline comments.

================
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:755
-  /// Describes the kind of result generated.
-  enum ResultKind {
-    /// Refers to a declaration.
----------------
egorzhdan wrote:
> kadircet wrote:
> > 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.
> 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136844/new/

https://reviews.llvm.org/D136844