[PATCH] D129556: [lld-macho] Fix loading same libraries from both LC_LINKER_OPTION and command line

Tue Jul 12 18:02:25 PDT 2022

int3 added a comment.

Nice fixes, thank you!

While looking back at my own diff, I noticed that one of the tests wasn't quite testing anything new... fixed in rG61ace8f78b1c52fd63fc8c2d800f08d7ddd87b0d <https://reviews.llvm.org/rG61ace8f78b1c52fd63fc8c2d800f08d7ddd87b0d>. You might want to pull in that fix and check that your changes pass (though I'm guessing they do).

================
Comment at: lld/MachO/Driver.cpp:274
+        forceLoadArchive == ForceLoad::Yes) {
+      currentScope = ArchiveFile::LoadScope::allLoad;
+    } else if (forceLoadArchive == ForceLoad::Default &&
----------------
I think this should work since LoadScope is just an `enum` and not an `enum class`?

================
Comment at: lld/MachO/Driver.cpp:1024-1027
+      // Do not force-load archives that are already loaded
+      if (loadedArchives.find(rerootPath(arg->getValue())) !=
+          loadedArchives.end())
+        forceload = ForceLoad::Default;
----------------
hmm why hoist this out of `addFile`? feels like it'd make sense to have all the archive load logic in one place

================
Comment at: lld/MachO/InputFiles.h:270-272
+    lazyLoad,
+    objCLoad,
+    allLoad,
----------------
our enums are usually capitalized

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

https://reviews.llvm.org/D129556