kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:97
+std::unique_ptr<SymbolIndex> connect(llvm::StringRef Address) {
+#ifdef CLANGD_REMOTE
+ return std::unique_ptr<SymbolIndex>(new IndexClient(Address));
----------------
sammccall wrote:
> if remote is disabled we can't compile the rest of this file.
> Rather than ifdefs here, I'd suggest doing it at the cmake level:
>
> ```
> if(CLANGD_ENABLE_REMOTE)
> add_clang_library(... Index.cpp)
> else()
> add_clang_library(... IndexUnimplemented.cpp)
> endif()
> ```
I would argue that including `index/remote/Client.h` without
`CLANGD_ENABLE_REMOTE` is not correct. We would still have to put `#ifdefs` in
the user code regardless of whether what you proposed is implemented. I don't
see any benefits in allowing users to include `index/remote/Client.h`, link
`clangdRemoteIndex` and getting a runtime error. All of those steps have
_remote_ in them and if _remote mode_ is not enabled, something certainly went
wrong.
Also, this will complicate CMake structure as I can't put files that are
conditionally added/excluded from the clang library and I would either have to
make a separate directory with an "empty" library, or put a bunch of `#ifdefs`
in here. Either is not optimal and I think it'd be better to leave it like
this. WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78521/new/
https://reviews.llvm.org/D78521
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
