[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/dex/dexp/Dexp.cpp:18
 #include "index/dex/Dex.h"
+#include "index/remote/Client.h"
 #include "llvm/ADT/ScopeExit.h"
----------------
sammccall wrote:
> Being able to include this header but not call the function in it doesn't make sense - linker errors aren't a friendly way to surface this constraint.
> 
> If the goal is to forbid use of the API without it compiled in, the header should `#ifndef #CLANGD_ENABLE_REMOTE #error "Client.h included but CLANGD_ENABLE_REMOTE is off" #endif`, and all the include-sites should be #ifdef-guarded.
> 
> (But please see other comments first)
Should be resolved in the newer version with (A) implementation.


================
Comment at: clang-tools-extra/clangd/index/remote/Client.h:22
+/// Caller is not blocked upon SymbolIndex request, the actual connection must
+/// happen upon the first request. RPCs have no timeout.
+///
----------------
sammccall wrote:
> RPCs have no timeout should be a FIXME
`streamRPC` already has a `FIXME` for that.


================
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:
> kbobyrev wrote:
> > 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?
> > 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
> 
> This is begging the question - what I'm proposing is to give that header/library defined behavior when CLANGD_ENABLE_REMOTE is off, and so you wouldn't need #ifdefs.
> 
> > I don't see any benefits in allowing users to include index/remote/Client.h, link clangdRemoteIndex and getting a runtime error.
> 
> The benefits are:
>  - no ifdefs and fewer conditional cmake rules, which are both hard to read and result in untested constructs without even basic syntax checking
>  - fewer cases to consider in clients, as this falls into the regular "can't connect to index" codepath
>  - readers trying to follow the possible behaviours end up at the header documentation of a function that is called, which is an easy and familiar workflow
> 
> > All of those steps have _remote_ in them and if _remote mode_ is not enabled, something certainly went wrong
> 
> Again, this is begging the question. If the header says "if GRPC mode is not enabled, always returns nullptr", then everything is working as designed.
> 
> > Also, this will complicate CMake structure as I can't put files that are conditionally added/excluded from the clang library
> 
> Why not? (This sounds like a fair technical objection, but may be surmountable)
I believe this is resolved in new version.

> > Also, this will complicate CMake structure as I can't put files that are conditionally added/excluded from the clang library
> Why not? (This sounds like a fair technical objection, but may be surmountable)
As discussed before, there is a CMake module in LLVM that errors out in case some files are not included in any library. In case of conditional compilation, either `UnimplementedClient.cpp` or `Client.cpp` will be left out (or this has to be the single file with `#ifdefs`). See new version for the exact layout I came up with.

This is especially unfortunate because I actually need two libraries - one for Client implementation and one for Marshalling. And because of that I have to have three separate directories :( Suboptimal, but probably OK.


================
Comment at: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt:17
 
-  protobuf
-  grpc++
-  clangDaemon
+  clangdRemoteIndex
   )
----------------
sammccall wrote:
> why does the server depend on the client?
Fixed in a newer diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78521