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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
