sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks, this looks good, just a couple of nits. I guess you don't have commit access, I'm happy to land this for you when you're happy with it. Just need an email address to associate the commit with. In D93600#2467393 <https://reviews.llvm.org/D93600#2467393>, @rapgenic wrote: > About the test, I thought it would be reasonable to test the three cases > separately, however I cannot get it to work with a single test file (I think > it's some problem with the generated files). Should I duplicate it or is > there any way to do it better? Yeah, these tests are a pain. I think you'd have to duplicate it, but honestly that's a maintenance trap, and it's really hard to factor out common logic in lit tests. I think folding in the complicated case into the existing test as you've done is probably best. ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:145 + + if (!llvm::sys::path::is_absolute(Driver)) { + auto DriverProgram = llvm::sys::findProgramByName(Driver); ---------------- maybe add an `assert` inside this if that there are no separators? ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:151 + Driver = *DriverProgram; + } + } ---------------- else log and return none? We don't want to continue if we couldn't find `clang` on the path. ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:346 llvm::SmallString<128> Driver(Cmd->CommandLine.front()); - llvm::sys::fs::make_absolute(Cmd->Directory, Driver); + if (!llvm::none_of(Driver, + [](char C) { return llvm::sys::path::is_separator(C); })) ---------------- nit: `!llvm::none_of` -> `llvm::any_of` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93600/new/ https://reviews.llvm.org/D93600 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
