[PATCH] D82252: MachO: support `-syslibroot`

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:136
+      for (auto root : roots) {
+        SmallString<261> buffer(root);
+        llvm::sys::path::append(buffer, path);
----------------
any particular reason why you picked 261?


================
Comment at: lld/MachO/Driver.cpp:147
+
+  // `-Z` supresses the standard "system" search paths.
+  if (args.hasArg(OPT_Z))
----------------
suppresses


================
Comment at: lld/test/MachO/syslibroot.test:1
+# Ensure that a non-existant path is ignored with a syslibroot
+
----------------
s/non-existant/nonexistent/


================
Comment at: lld/test/MachO/syslibroot.test:3
+
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty | FileCheck %s -check-prefix CHECK-NONEXISTANT-SYSLIBROOT
+
----------------
s/NONEXISTANT/NONEXISTENT/


================
Comment at: lld/test/MachO/syslibroot.test:14
+
+RUN: mkdir -p %t/Users/compnerd/Library/libxml2-development
+RUN: lld -flavor darwinnew -v -syslibroot %t -L /Users/compnerd/Library/libxml2-development | FileCheck %s -check-prefix CHECK-ABSOLUTE-PATH-REROOTED
----------------
nit: could we have a shorter tmp directory name?


================
Comment at: lld/test/MachO/syslibroot.test:15
+RUN: mkdir -p %t/Users/compnerd/Library/libxml2-development
+RUN: lld -flavor darwinnew -v -syslibroot %t -L /Users/compnerd/Library/libxml2-development | FileCheck %s -check-prefix CHECK-ABSOLUTE-PATH-REROOTED
+
----------------
nit: most other tests use double-dash flags, i.e. `--check-prefix`


================
Comment at: lld/test/MachO/syslibroot.test:18
+CHECK-ABSOLUTE-PATH-REROOTED: Library search paths:
+CHECK-ABSOLUTE-PATH-REROOTED: {{.+}}/Users/compnerd/Library/libxml2-development
+CHECK-ABSOLUTE-PATH-REROOTED: {{.+}}/usr/lib
----------------
nit: would be better to pass the tmp dir to FileCheck via `-DTMP=%t` and use it here instead of `{{.+}}`. I believe the current check would pass even if there were only whitespace before `/Users/...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82252