[PATCH] D61895: Introduce CommonSymbol.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 15 04:16:56 PDT 2019


grimar added inline comments.


================
Comment at: lld/ELF/SymbolTable.cpp:136
+
+//
+void SymbolTable::mergeProperties(Symbol *Old, Symbol *New) {
----------------
Missing comment.


================
Comment at: lld/ELF/SymbolTable.cpp:295
+
+  if (OldSym->Section == nullptr && NewSym->Section == nullptr &&
+      OldSym->Value == NewSym->Value && NewSym->Binding == STB_GLOBAL)
----------------
Not strong opinion, but perhaps `!OldSym->Section && !NewSym->Section` would be better (while you are here).


================
Comment at: lld/ELF/SymbolTable.cpp:320
 
-  if (Config->WarnCommon)
-    warn("multiple common of " + D->getName());
+  auto *OldSym = cast<CommonSymbol>(Old);
 
----------------
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?


================
Comment at: lld/ELF/Symbols.h:254
+// section. (Therefore, the later passes don't see any CommonSymbols.)
+class CommonSymbol : public Symbol {
+public:
----------------
delcarations -> declarations
Neverthless -> Nevertheless
varaible -> variable

(actually this is my spell checker who complains, I was fine with the original version :])


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61895





More information about the llvm-commits mailing list