[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge updated this revision to Diff 471925. nridge added a comment. reformulate test as lit test address other review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133757/new/ https://reviews.llvm.org/D133757 Files: clang-tools-extra/c

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

sammccall added a comment. I think this patch is great apart from the testing issue. (I do see the value in a good end-to-end test for the exact bug being fixed, but would also be fine with this being a unit-test of CommandMangler with the query driver part mocked out) Comme

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge updated this revision to Diff 462704. nridge marked 2 inline comments as done. nridge added a comment. This is mostly just a rebase on top of the update to D133756 to turn CompileCommandsAdjuster into a unique_function, and addressing a few minor comments

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

kadircet added a comment. In D133757#3792930 , @nridge wrote: > Thanks for the feedback! Yeah I'd be wary of introducing a corner case where > the user passes `--query-driver`, and there is in fact a driver available to > query in `PATH`, but we end up

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge added a comment. Thanks for the feedback! Yeah I'd be wary of introducing a corner case where the user passes `--query-driver`, and there is in fact a driver available to query in `PATH`, but we end up not querying it. Even if the outcome is the same in cases we can think of, it feels li

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

sammccall added a comment. Kadir and I discussed this a bunch today. FWIW I agree with this patch: sticking the query-driver step in the middle of the pipeline is the right way to solve these bugs. Putting it on the end and hoping to get away with it feels risky to me. Commen

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

kadircet added a comment. In D133757#3791485 , @nridge wrote: > I think the sticking point for just having `QueryDriverDatabase` run after > the entirety of `CommandMangler` is this check >

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge added a comment. I think the sticking point for just having `QueryDriverDatabase` run after the entirety of `CommandMangler` is this check in `resolveDriver()`,

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

kadircet added a comment. thanks, i agree that we should make query driver work coherently with `CompileFlags.Compiler`, but I am not sure if we really need to complexity for handling this adjustment logic before resolving the driver. I've got some comments right next to the change but to summa

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge added inline comments. Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:242 +CompileFlags: + Compiler: gcc + )yaml"; To exercise `QueryDriverDatabase`, this test assumes that the machine running the test will have a `gcc` somewher

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge added a comment. So far I've only written a test for https://github.com/clangd/clangd/issues/1089. I plan to add tests for the other two issues as well, but I thought I'd get the review ball rolling in the mean time to get feedback on the fix approach and test approach. Repository:

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge added a comment. Depends on https://reviews.llvm.org/D133756 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133757/new/ https://reviews.llvm.org/D133757 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

nridge created this revision. nridge added reviewers: kadircet, sammccall. Herald added a subscriber: arphaman. Herald added a project: All. nridge requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. This allo