[PATCH] D61895: Introduce CommonSymbol.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu marked 2 inline comments as done.
ruiu added inline comments.


================
Comment at: lld/ELF/SymbolTable.cpp:155
+  }
 
   // An undefined symbol with non default visibility must be satisfied
----------------
MaskRay wrote:
> `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).
Currently the first symbol needs to be merged with itself. I'll try to fix later.


================
Comment at: lld/ELF/SymbolTable.cpp:320
 
-  if (Config->WarnCommon)
-    warn("multiple common of " + D->getName());
+  auto *OldSym = cast<CommonSymbol>(Old);
 
----------------
grimar wrote:
> Lets use `CommonSymbol` instead of `auto`? `OldSym` has an obvious type here, but I feel sometimes that `auto` is something
> that was invented for iterators or stuff like that and often used a bit too often than needed. 
> 
> I.e. I think it should be used when the type is so complex in spelling that you should not think about it. Otherwise for readablility,
> like in this place, probably it is better just to use the original type name. What do you think?
I'm fine in either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61895