[PATCH] D78521: [clangd] Extend dexp to support remote index

[Kirill Bobyrev via Phabricator via cfe-commits]

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