[PATCH] D83532: [lld-macho] Partial support for weak definitions

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: lld/MachO/ExportTrie.cpp:63
+  uint8_t flags;
+  ExportInfo(const Symbol &sym)
+      : address(sym.getVA()),
----------------
Can you please mark this `explicit`?


================
Comment at: lld/MachO/ExportTrie.h:40
 using TrieEntryCallback =
-    llvm::function_ref<void(const llvm::Twine & /*name*/, uint64_t /*flags*/)>;
+    llvm::function_ref<void(const llvm::Twine & /*name*/, bool /*isWeakDef*/)>;
 
----------------
Are we sure we wont need any other flags?  I wonder if it's better to just treat weakness as a flag.  IIRC, there is a `EXPORT_SYMBOL_FLAGS_REEXPORT` and `EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL` that would be fairly good to account for.


================
Comment at: lld/MachO/InputFiles.cpp:235
+      return make<Defined>(name, isec, value, sym.n_desc & N_WEAK_DEF);
   };
 
----------------
It's okay if you want to use braces, but please use them on both sides.  However, I think that this is better written as:

```
  if (sym.n_type & N_EXT)
    return symtab->addDefined(name, isec, value, sym.n_desc & N_WEAK_DEF);
  return make<Defined>(name, isec, value, sym.n_desc & N_WEAK_DEF);
```


================
Comment at: lld/MachO/SymbolTable.cpp:52
+    }
+  }
 
----------------
What do you think of doing an early return instead if the symbol was inserted?

Can you explain why currently it is not an error if the symbol is not inserted and not defined?  Seems like a comment for that would be good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532