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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to