[PATCH] D62442: [Driver] Update handling of c++ and runtime directories

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D62442#1517183 <https://reviews.llvm.org/D62442#1517183>, @mcgrathr wrote:

> lgtm contingent on verifying intended behavior changes and adding comments.  The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.


I have undone that factoring as I have realized I could simplify the logic further, but as that's going to require more cleanup I'll do it in a follow up change.



================
Comment at: clang/lib/Driver/ToolChain.cpp:389
     SmallString<128> P(LibPath);
     llvm::sys::path::append(P, Prefix + Twine("clang_rt.") + Component + Suffix);
+    return P.str();
----------------
mcgrathr wrote:
> Can you explain the change to  use the first entry rather than the first existing entry?
> If the first one in the list is presumed to exist, then why is there a list instead of a single directory stored?
That was unintentional, I've reverted that change.


================
Comment at: clang/lib/Driver/ToolChain.cpp:424
+
+Optional<std::string> ToolChain::getCXXStdlibPath() const {
+  SmallString<128> P;
----------------
mcgrathr wrote:
> Same here.  Can these use a common subroutine/template for trying something based on these two, so the logic about what D.getTargetTriple() and Triple.str() mean and both the code and justifying comments for their ordering logic is de-duplicated?
I plan on cleaning up the runtime handling in a follow up change.


================
Comment at: clang/test/Driver/fuchsia.c:96
 // CHECK-ASAN-X86: "-dynamic-linker" "asan/ld.so.1"
-// CHECK-ASAN-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib{{/|\\\\}}asan"
-// CHECK-ASAN-X86: "-L[[RESOURCE_DIR]]{{/|\\\\}}x86_64-fuchsia{{/|\\\\}}lib"
----------------
mcgrathr wrote:
> To clarify, these are dropped because their sole intent was always to serve -lc++ and that's properly not enabled for this C-only link?
Correct.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62442