kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/CompileCommands.cpp:145 + // Otherwise try to look it up on PATH. This won't change basename. + if (auto Absolute = llvm::sys::findProgramByName(Driver)) + Driver = Storage = *Absolute; ---------------- `findProgramByName` is evil :/ it is not guaranteed to return an absolute path. for example if you've got `toolchain/clang` as your argv[0] (not sure if it is possible, but it is a non-absoltue path...) then it will return the argument directly, even though it is not an absolute path :(. ================ Comment at: clang-tools-extra/clangd/CompileCommands.cpp:156 + llvm::SmallString<256> Resolved; + if (llvm::sys::fs::real_path(Driver, Resolved)) + return Driver.str(); ---------------- what about taking a VFS instead and calling `VFS.getRealPath`? It should make testing easier and commandmangler vfs friendly. ================ Comment at: clang-tools-extra/clangd/CompileCommands.h:48 CommandMangler() = default; + Memoize<llvm::StringMap<std::string>> CachedResolveDriver; }; ---------------- maybe just `ResolvedDriver`? ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:286 + // Caches includes extracted from a driver. Key is driver:lang. + Memoize<llvm::StringMap<std::vector<std::string>>> DriverToIncludesCache; + llvm::Regex QueryDriverRegex; ---------------- nit: `QueriedDrivers` ================ Comment at: clang-tools-extra/clangd/Threading.h:154 + template <typename T, typename Func> + typename Container::mapped_type operator()(T &&Key, Func Compute) const { + { ---------------- what about an explicit `getOrCreate` method instead? ================ Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:140 + std::vector<std::string> Cmd = {(TempDir + "/bin/foo").str(), "foo.cc"}; + Mangler.adjust(Cmd); + // Directory based on resolved symlink, basename preserved. ---------------- irrelevant to the patch: any reason for `adjust` to mutate its parameter in-place instead of returning it(I believe callers can do `std::move(Cmd)` if need be) ? ================ Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:144 + + // Set PATH to point to temp/bin so we can find 'foo' on it. + ASSERT_TRUE(::getenv("PATH")); ---------------- can we rather move the `ifdef` here the rest previous part should hopefully work on other platforms as well (at least after VFS)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75414/new/ https://reviews.llvm.org/D75414 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits