[PATCH] D61895: Introduce CommonSymbol.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/SymbolTable.cpp:155
+  }
 
   // An undefined symbol with non default visibility must be satisfied
----------------
`mergeProperties(Old, New);` can be moved after `if (Old->isPlaceholder()) {`

See my question in D61855. I think we should decrease the number of redundant assignments (if possible).


================
Comment at: lld/ELF/SymbolTable.cpp:156
 
   // An undefined symbol with non default visibility must be satisfied
   // in the same DSO.
----------------
Not the fault of this patch, but this comment looks obscure..

`isa<SharedSymbol>(Old)` -> `Old->isShared()`.


================
Comment at: lld/ELF/SymbolTable.cpp:496
     for (Symbol *Sym : SymVector) {
       if (!Sym->isDefined())
         continue;
----------------
`getDemangledSyms` is transitively called by `scanVersioScript` before `replaceCommonSymbols`. Is a `isCommon()` check needed here?


================
Comment at: lld/ELF/SymbolTable.cpp:511
   if (Symbol *B = find(Ver.Name))
     if (B->isDefined())
       return {B};
----------------
Ditto.  Is a isCommon() check needed here?


================
Comment at: lld/ELF/SymbolTable.cpp:528
   for (Symbol *Sym : SymVector)
     if (Sym->isDefined() && M.match(Sym->getName()))
       Res.push_back(Sym);
----------------
Ditto.  Is a isCommon() check needed here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61895