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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/SymbolTable.cpp:52
+    }
+  }
 
----------------
int3 wrote:
> compnerd wrote:
> > int3 wrote:
> > > compnerd wrote:
> > > > 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.
> > > not inserted and not defined => we will fall through to replaceSymbol below which will replace any Undefined/Lazy/Dylib symbols with the Defined one
> > I think that my confusion here is that the not inserted implies that it already exists, but because the cast to Defined failed, it cannot be a defined symbol, so you are replacing it with an undefined symbol or or something else?  Do we know what is replacing the symbol?  If it is not defined, then why is this function called `addDefined`?
> the cast to Defined failing means that the existing symbol isn't Defined, and so we're replacing it with a Defined symbol. I'm not sure why you think we may be replacing it with an undefined symbol, but note that `replaceSymbol` is changing the value and type of `s` via `placement new` (maybe you thought it was inserting `s`?)
Ah, now I follow.  I think it would be nice to have that as a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532