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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
