Thanks for looking!
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...):
This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the clangd binary in a very specific configuration (no flags will be plumbed through).
Is it actually important that the clangd binary itself can be used with XPC, rather than defining another binary that spawns a ClangdServer+XPCTransport with the right config? If you strip out all of ClangdMain that's not related to flag parsing and options, there's almost nothing left...
tool/ClangdMain.cpp | ||
---|---|---|
30 | WDYT about putting newXPCTransport() in Transport.h behind an ifdef? That will be consistent with JSON, allow the XPCTransport.h to be removed entirely, and remove one #ifdef here. I find the #ifdefs easier to understand if they surround functional code, rather than #includes - might be just me. | |
328 | I'd consider putting this outside the #ifdef, so we can print an error and exit if it's requested but not built. In fact, can this just be a regular flag instead? -transport={xpc|json} | |
329 | no support for -input-mirror or pretty-printing in the logs - deliberate? | |
xpc/CMakeLists.txt | ||
21 | why is conversions a separate library from transport? No problem with fine-grained targets per se, but it's inconsistent with much of the rest of clang-tools-extra. | |
xpc/Conversion.cpp | ||
16 | nit: using namespace llvm; namespace clang { namespace clangd { (we're finally consistent about this) | |
22 | Key, PayloadString, etc | |
23 | nit: you can #include ScopedPrinter, and then this is just llvm::to_string(json) | |
28 | ah, this encoding is a little sad vs the "native" encoding of object -> xpc_dictionary, array -> xpc_array etc which looked promising... Totally your call, but out of curiosity - why the change? | |
xpc/Conversion.h | ||
19 | param name json -> JSON | |
xpc/XPCTransport.cpp | ||
142 | lowercase namespace? | |
177 | I'm not clear on the relationship between XPC services and connections - if there are multiple connections, what happens? Do you want to check and close any subsequent ones? Can connections and/or messages arrive on multiple threads? | |
192 | you could consider asserting that they were previously null | |
xpc/XPCTransport.h | ||
19 | as mentioned above, I think this may be more discoverable in Transport.h, with ifdef. |
I don't really expect you to be familiar with XPC - feel free to ask about anything and I'll do my best explaining.
It's more like that configuration isn't implemented in this initial patch but as we can't use command line options (launchd doesn't pass these to XPC service) we'll have to duplicate these as environment variables. We discusse internally and we prefer to have just single binary as that would make integration and testing easier for us.
tool/ClangdMain.cpp | ||
---|---|---|
30 | Ok, sounds reasonable. | |
328 | Unfortunately it's not possible to have launchd pass flags when spawning XPC services (otherwise it would be my first choice too). | |
329 | I was thinking this could be added later. | |
xpc/CMakeLists.txt | ||
21 | Guilty of YAGNI violation here - was thinking about eventual use by XPC clients. I'll stick to consistency for now and would separate it if needed. | |
xpc/Conversion.cpp | ||
16 | Ok. Thanks. | |
22 | Thanks. Somehow I was always working on projects with different naming conventions and still struggle with this. | |
23 | I'll take a look at ScopedPrinter. Thanks. | |
28 | I know. At first we didn't even think of anything else than 1:1 mapping between JSON and XPC dictionary as it's just natural. But later we learned about performance issues with construction of XPC dictionaries with lots of elements which would be a problem for code completion. | |
xpc/Conversion.h | ||
19 | Good catch. Thanks. | |
xpc/XPCTransport.cpp | ||
142 | Right. I should probably start looking for some tool checking this... | |
177 | Our use-case is an XPC service which is bundled to an application. It could be described as a "private daemon" - the service is launched on-demand for the application. There should be either zero or one connection at a time. As for messages - we didn't plan for concurrent use. I'll add explicit synchronization. | |
192 | Good idea. | |
xpc/XPCTransport.h | ||
19 | I agree, will move it. |
Since AFAIK Sam is off until the end of the year I am adding more reviewers.
tool/ClangdMain.cpp | ||
---|---|---|
329 | Just to clarify - since we'd like to have just a single binary for XPC and stdio transport and we sadly can't use command line options we'd have to duplicate probably all of them as env variables. | |
xpc/CMakeLists.txt | ||
21 | Now that I actually took a better look we already have one client - our xpc-test-client. I assume that means - it's fine as it is, right? | |
xpc/XPCTransport.cpp | ||
177 | I want to make the synchronous operation explicit here. Still need to work on this. |
This patch seems to have broken the compilation. I get the following error on a linux platform:
[12/14] Linking CXX executable bin/clangd
FAILED: bin/clangd
: && /usr/local/google/home/tamur/src/llvm/2018_nov_12/llvm/Stable/bin/clang++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -fuse-ld=lld -Wl,--color-diagnostics -Wl,-allow-shlib-undefined -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -o bin/clangd -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.so.8svn -lpthread lib/libclangBasic.so.8svn lib/libclangDaemon.so.8svn lib/libclangFormat.so.8svn lib/libclangFrontend.so.8svn lib/libclangSema.so.8svn lib/libclangTooling.so.8svn lib/libclangToolingCore.so.8svn && :
ld.lld: error: undefined symbol: clang::clangd::newXPCTransport()
Sorry for not getting back to this, but it looks good.
I'm really glad about how minimal the changes are outside xpc/, because it makes it less likely we'll accidentally break something that can only be tested on mac.
WDYT about putting newXPCTransport() in Transport.h behind an ifdef?
That will be consistent with JSON, allow the XPCTransport.h to be removed entirely, and remove one #ifdef here.
I find the #ifdefs easier to understand if they surround functional code, rather than #includes - might be just me.