[PATCH] D116913: [lld-macho] Add --start-lib --end-lib

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

So I realize that the choice of class hierarchy here was chosen to follow the LLD-ELF backend's design... but I'm not the biggest fan of it for two reasons:

1. `ObjFile`s and `BitcodeFile`s with `lazy = true` aren't proper instances of their classes; e.g. calling `->getDataInCode()` on a lazy ObjFile isn't a valid operation. IMO it would make more sense to have a separate LazyFile instance that gets converted to ObjFile / BitcodeFile as necessary. Kind of like an ArchiveFile but with only one member.
2. I have the opposite objection to `LazyObject`: it doesn't really merit the creation of a new class. Implementation-wise, it's indeed different from `LazyArchive`, but interface-wise, they both expose a single common operation: the fetching of their associated file. IMO it would be cleaner to have a single `LazySymbol` class with a `fetchFile()` method. It can use a PointerUnion to store a pointer to the `llvm::object::Archive::Symbol` or InputFile instance as necessary.

All that said, I do see the sense in not deviating from LLD-ELF's architecture. If you prefer the hierarchy as-is, I'm good with that too.



================
Comment at: lld/MachO/Driver.cpp:350
   if (newFile && !isa<DylibFile>(newFile)) {
+    if ((magic == file_magic::macho_object || magic == file_magic::bitcode) &&
+        newFile->lazy && config->forceLoadObjC) {
----------------
nit: how about `isa<ObjFile>` and `isa<BitcodeFile>` since the line above is also using `isa<>`?


================
Comment at: lld/MachO/Driver.cpp:350-359
+    if ((magic == file_magic::macho_object || magic == file_magic::bitcode) &&
+        newFile->lazy && config->forceLoadObjC) {
+      for (Symbol *sym : newFile->symbols)
+        if (sym && sym->getName().startswith(objc::klass)) {
+          extract(*newFile, "-ObjC");
+          break;
+        }
----------------
int3 wrote:
> nit: how about `isa<ObjFile>` and `isa<BitcodeFile>` since the line above is also using `isa<>`?
IIUC, we are evaluating lazy object files for ObjC members after doing regular symbol resolution. Archive loading (intentionally) does this in the opposite order: {D108781}. Would probably be good to follow suit


================
Comment at: lld/MachO/InputFiles.cpp:929
+  symbols.resize(nList.size());
+  for (auto it : llvm::enumerate(nList)) {
+    const NList &sym = it.value();
----------------
nit: no need for `llvm::`


================
Comment at: lld/MachO/InputFiles.cpp:1619
+  symbols.resize(obj->symbols().size());
+  for (auto it : llvm::enumerate(obj->symbols())) {
+    const lto::InputFile::Symbol &objSym = it.value();
----------------
no need for `llvm::`


================
Comment at: lld/MachO/InputFiles.h:242
+  void parse();
+  void parseLazy();
 
----------------
make this private?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116913