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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Thanks for the revision, but I still think this is more complicated than it needs to be. I think maybe my initial comment was not clear enough... in particular

> there isn't a need to calculate and store a LoadScope as isCommandLineLoad should be enough.

i.e. we don't need the LoadScope enum at all. I've now suggested the code change to show how it would work...

> Tell me if you think it's better to store LoadFrom in loadedArchives and I will change it, thanks!

I think it's better because we will never access it outside of Driver.cpp. So no need for it to be in the header. Also, from my previous comment:

> Storing bool isCommandLineLoad rather than LoadScope makes it clear (from looking at the data structures alone) that our cache update policy depends only on whether something is being loaded from LC_LINKER_OPTION vs the CLI

i.e. could store just a boolean instead of the LoadFrom enum. This one is not such a big deal, if you prefer storing the enum that is fine too



================
Comment at: lld/MachO/Driver.cpp:272-316
+    // -all_load & -ObjC have an effect on command line arguments
+    if (loadFrom == LoadFrom::CLILazy && config->allLoad) {
+      loadFrom = LoadFrom::AllLoad;
+    } else if (loadFrom == LoadFrom::CLILazy && config->forceLoadObjC) {
+      loadFrom = LoadFrom::ObjCLoad;
+    }
+
----------------



================
Comment at: lld/MachO/Driver.cpp:318
 
-    auto *file = make<ArchiveFile>(std::move(archive));
-    if ((forceLoadArchive == ForceLoad::Default && config->allLoad) ||
-        forceLoadArchive == ForceLoad::Yes) {
+    if (loadScope == LoadScope::All) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
----------------



================
Comment at: lld/MachO/Driver.cpp:332
       }
-    } else if (forceLoadArchive == ForceLoad::Default &&
-               config->forceLoadObjC) {
+    } else if (loadScope == LoadScope::ObjC) {
       for (const object::Archive::Symbol &sym : file->getArchive().symbols())
----------------



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

https://reviews.llvm.org/D129556