[PATCH] D126289: [Clang][Driver] Fix include paths for `--sysroot /` on Linux

[Fangrui Song via Phabricator via cfe-commits]

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