MaskRay added inline comments.
================ Comment at: clang/include/clang/Driver/ToolChain.h:219 + + static std::string concat(const std::string &Path, const Twine &A, + const Twine &B = "", const Twine &C = "", ---------------- egorzhdan wrote: > MaskRay wrote: > > This can use `llvm::sys::path::append` > The implementation of this already uses `llvm::sys::path::append`, do you > mean replacing the usages of `concat` with `llvm::sys::path::append`? > That would produce quite a lot of boilerplate I think, since > `llvm::sys::path::append` modifies the path in-place, so every call to > `concat` would expand into 3 lines of code. Also it would be very easy to > forget to pass `Style::posix` and cause incorrect behavior on Windows > (something I've stumbled upon while making this patch). Ah I missed that. The current `concat` impl looks good to me. ================ Comment at: clang/lib/Driver/ToolChains/Linux.cpp:277 - addPathIfExists(D, SysRoot + "/lib/" + MultiarchTriple, Paths); - addPathIfExists(D, SysRoot + "/lib/../" + OSLibDir, Paths); + addPathIfExists(D, concat(SysRoot, "/lib/", MultiarchTriple), Paths); + addPathIfExists(D, concat(SysRoot, "/lib/../", OSLibDir), Paths); ---------------- Can the trailing `/` in a path component be removed now? Ditto below. ================ Comment at: clang/test/Driver/linux-header-search.cpp:99 +// CHECK-BASIC-LIBSTDCXX-SYSROOT-SLASH: "-cc1" +// CHECK-BASIC-LIBSTDCXX-SYSROOT-SLASH: "-isysroot" "[[SYSROOT:[^"]+/]]" +// CHECK-BASIC-LIBSTDCXX-SYSROOT-SLASH: "-internal-isystem" "[[SYSROOT]]usr/lib/gcc/x86_64-unknown-linux-gnu/4.8/../../../../x86_64-unknown-linux-gnu/include" ---------------- add `-SAME` whenever applicable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126289/new/ https://reviews.llvm.org/D126289 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits