sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks! I understand the clangd stuff better now, and scanning through the other changes they seem to be the same pattern. LGTM ================ Comment at: clang-tools-extra/clangd/CMakeLists.txt:105 + +clang_target_link_libraries(clangDaemon + PRIVATE ---------------- mgorny wrote: > sammccall wrote: > > This has split our link dependencies in two, and I'm not really sure why > > (and so am likely to put future dependencies in the wrong place). > > > > Can *all* the link dependencies be moved to the clang_target_link_libraries > > section? > > ((Even if this addresses a problem only seen with clang libs, I'd rather > > have everything in one place) > No. `clang_target_link_libraries` is only for libraries that are included in > `libclang-cpp.so`, so when the latter is built, they are replaced with it > entirely. I'm all for improving this (e.g. to automatically replace only > these libs that are included in clang-cpp) but I don't really have time to > work on this beyond fixing immediate failures. ah, thanks. Sounds like it should be `target_link_clang_libraries` then :-) ================ Comment at: clang-tools-extra/clangd/unittests/CMakeLists.txt:123 clangTidy - LLVMSupport LLVMTestingSupport ---------------- mgorny wrote: > sammccall wrote: > > why this change? We do depend directly on LLVMSupport, and I'd prefer that > > to remain explicit rather than pick it up transitively. > It's already listed in `LLVM_LINK_COMPONENTS` which handles using libLLVM > correctly. Listing it here results in another copy of LLVMSupport being > linked in which caused the test to crash immediately on duplicate > command-line options. Thanks, sounds like we should move TestingSupport there too (but will do that separately). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81967/new/ https://reviews.llvm.org/D81967 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
