[PATCH] D80677: [lld-macho] Handle framework search path, alongside library search path

[Greg McGary via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/Config.h:31
+  std::vector<llvm::StringRef> librarySearchPaths;
+  std::vector<llvm::StringRef> frameworkSearchPaths;
   llvm::DenseMap<llvm::StringRef, SymbolPriorityEntry> priorities;
----------------
int3 wrote:
> maybe add a TODO to indicate this isn't currently used
Si, señor


================
Comment at: lld/MachO/Driver.cpp:103
+static void checkSearchDirs(StringRef option, std::vector<StringRef> &paths) {
+  for (const auto &path : paths) {
+    if (!fs::exists(path))
----------------
int3 wrote:
> not a big deal, but how about erasing the invalid paths from the list?
> 
> though if we aren't doing that we should take `paths` by const ref at least
You are quite correct that it is bone-headed to keep excreta in the search path.


================
Comment at: lld/test/MachO/search-paths.test:6
+RUN: lld -flavor darwinnew -v -L%t1 -L %t2 -L%t3 -L %t4 -F%t5 -F %t6 -F%t7 -F %t8 2>&1 \
+RUN: | FileCheck -DT1=%t1 -DT2=%t2 -DT3=%t3 -DT4=%t4 -DT5=%t5 -DT6=%t6 -DT7=%t7 -DT8=%t8 --check-prefix=CHECK_noZ %s
+
----------------
int3 wrote:
> nit: do we really need 8 paths? wouldn't a test with one or two library/framework paths respectively be sufficient?
I got kinda carried away. I will show more restraint ... 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80677