[PATCH] D70306: clang: Exherbo multiarch ajustments

[Marc-Antoine Perennou via Phabricator via cfe-commits]

Keruspe planned changes to this revision.
Keruspe marked 3 inline comments as done.
Keruspe added a comment.

Will give llvm::sys::path::stem a look and try to drop the x86_64 part from the include dir selection



================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:360
+      addPathIfExists(D,
+                      LibPath + "/../../" + GCCTriple.str() + "/lib/../" +
+                          OSLibDir + SelectedMultilib.osSuffix(),
----------------
compnerd wrote:
> Could you use `llvm::sys::path::stem` please?
This was just a copy of the code that as already there, with a different path, but I sure can give that a look


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:889
+      break;
+    }
+  }
----------------
compnerd wrote:
> Why not just always construct the path?  `"/usr/" + getTriple.str() + "/include"` is always going to be the path that we have for exherbo
That's what we had at first, but then e.g. `clang -m32` didn't work as it was searching inside of i383 and not i686, and wrt x86_64 it was using unknown at some point instead of pc.
The x86_64 part might have been because it was in another environment but the x86 part definitely is still relevant


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:984
+  if (Distro == Distro::Exherbo &&
+      addLibStdCXXIncludePaths(
+          LibDir.str() + "/../../" + TripleStr + "/include",
----------------
compnerd wrote:
> Why is this part of the `if`?  This should be part of the code executed conditionally.
It just matches the if from below, with the distro check added. Not familiar with the llvm codebase so I'm trying to just stick to the similar code that's just next to it


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70306/new/

https://reviews.llvm.org/D70306