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

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: clang/lib/Driver/Driver.cpp:4428
 
-  if (auto P = SearchPaths(TC.getFilePaths()))
+  if (auto P = SearchPaths(TC.getRuntimePaths()))
     return *P;
----------------
I see the renaming Library->Runtime throughout (not clear why, but OK by me).  This also swaps the order of getFilePaths vs getRuntimePaths, which seems significant.  There's a woeful lack of comments in this function about its essential logic, which is the order in which it consults all the available sources of paths.
So changing the ordering a bit without any comments is concerning.

I'd like to see more comments overall, but as this is a preexisting problem in this code, it's enough for this change that you affirm this ordering change was intentional and add some commentary somewhere about the material change in behavior (here or in the log message).


================
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();
----------------
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?


================
Comment at: clang/lib/Driver/ToolChain.cpp:410
+  SmallString<128> P;
+
+  P.assign(D.ResourceDir);
----------------
Comment about the distinction between D.getTargetTriple() and Triple.str() and why this order between them.


================
Comment at: clang/lib/Driver/ToolChain.cpp:424
+
+Optional<std::string> ToolChain::getCXXStdlibPath() const {
+  SmallString<128> P;
----------------
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?


================
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"
----------------
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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62442