[PATCH] D62117: Make replaceSymbol a member function of Symbol.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/trunk/ELF/Symbols.h:86
 
-  // Symbol binding. This is not overwritten by replaceSymbol to track
+  // Symbol binding. This is not overwritten by replace() to track
   // changes during resolution. In particular:
----------------
smeenai wrote:
> Sorry for commenting on an old diff.
> 
> I'm trying to understand this comment and if it's still true. As far as I can tell, `[replace()](https://github.com/llvm/llvm-project/blob/204d301bb1921431a853c0bfba32007c018df1d5/lld/ELF/Symbols.h#L529)` is just doing a `memcpy` from the new symbol into the old symbol, and the `binding` field isn't restored afterwards, so `replace` will in fact overwrite the binding, right?
> 
> I imagine this comment is from a time when the behavior was different (particularly when SymbolBody used to be distinct from Symbol and the binding was part of Symbol and not SymbolBody), but it doesn't seem to be accurate anymore, as far as I can tell.
`This is not overwritten by replace()` is indeed misleading. `replace` overrides the binding. The only weird place is undefined weak + un-extracted archive member which is represented as a weak lazy symbol which is considered `isUndefWeak`.

`An undefined weak is still weak when it resolves to a shared library.` is normal rule which probably doesn't deserve a dedicated comment here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62117