[PATCH] D81124: lld: use modern library search ordering

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd marked 2 inline comments as done.
compnerd added inline comments.


================
Comment at: lld/MachO/Driver.cpp:85
+      llvm::sys::path::append(location, library);
+      if (fs::exists(location))
+        return location.str().str();
----------------
gkm wrote:
> compnerd wrote:
> > gkm wrote:
> > > Nit: check for `fs::is_regular_file()`
> > That was not checked for previously, why introduce that with this change? 
> It's an opportunistic improvement: a tighter validity check.
Seems better as a separate change as it technically changes the semantics.


================
Comment at: lld/test/MachO/link-search-order.s:16
+# CHECK: /usr/lib/libSystem.B.dylib
+
+.section __TEXT,__text
----------------
gkm wrote:
> compnerd wrote:
> > gkm wrote:
> > > Please add an archive that will be found, alongside the one that is shadowed by a dylib.
> > I think that the existing check for the static archive covers that part of the test.  Arguably, because this change is about cleaning up that path, it _could_ make sense to remove the test case from the change and do the testing improvements later.  What case is it that is missing testing?
> Tested:
> * use a dylib (does not shadow an archive)
> * use a dylib (shadows an archive)
> Untested:
> * use an archive (not shadowed by a dylib)
The last case already tested in a different test that I added previously.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81124