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

[Saleem Abdulrasool via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/Driver.cpp:79
+  std::string archive = (llvm::Twine("lib") + name + ".a").str();
+  llvm::SmallString<260> location;
 
----------------
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.


================
Comment at: lld/MachO/Driver.cpp:85
+      llvm::sys::path::append(location, library);
+      if (fs::exists(location))
+        return location.str().str();
----------------
gkm wrote:
> Nit: check for `fs::is_regular_file()`
That was not checked for previously, why introduce that with this change? 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81124