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

[Greg McGary via Phabricator via llvm-commits]

gkm added inline comments.


================
Comment at: lld/MachO/Driver.cpp:79
+  std::string archive = (llvm::Twine("lib") + name + ".a").str();
+  llvm::SmallString<260> location;
 
----------------
compnerd wrote:
> MaskRay wrote:
> > gkm wrote:
> > > Nit: I'd rather see `PATH_MAX`, but literal ints like this are the norm.
> > Do we omit `llvm::` for commonly used LLVM ADT/Support classes?
> `PATH_MAX` is different from `MAX_PATH` ... 260 is the larger of the two and there is no macro to unify the two spellings on different platforms.
Very well.


================
Comment at: lld/MachO/Driver.cpp:85
+      llvm::sys::path::append(location, library);
+      if (fs::exists(location))
+        return location.str().str();
----------------
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.


================
Comment at: lld/test/MachO/link-search-order.s:16
+# CHECK: /usr/lib/libSystem.B.dylib
+
+.section __TEXT,__text
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81124