egorzhdan added inline comments.
================
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:755
- /// Describes the kind of result generated.
- enum ResultKind {
- /// Refers to a declaration.
----------------
kadircet wrote:
> 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.
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136844/new/
https://reviews.llvm.org/D136844
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
