[PATCH] D19752: ELF: New symbol table design.

[Rui Ueyama via llvm-commits]

ruiu added a comment.

This is generally looking good. First round of review comments.


================
Comment at: ELF/InputFiles.h:126-127
@@ -122,3 +125,4 @@
   explicit ObjectFile(MemoryBufferRef M);
-  void parse(llvm::DenseSet<StringRef> &ComdatGroups);
+  void parse(SymbolTable<ELFT> *Symtab,
+             llvm::DenseSet<StringRef> &ComdatGroups);
 
----------------
I think it's better to make Symtab globally accessible just like Config object. It is a singleton, and it is accessed everywhere (Writer is using it, symbol resolution of course uses it, and now file parsing is starting to use it.)

================
Comment at: ELF/InputFiles.h:190
@@ -185,5 +189,3 @@
 
-  void parse();
-
-  llvm::MutableArrayRef<LazyObject> getLazySymbols() { return LazySymbols; }
+  void parse(SymbolTableBase *Symtab);
 
----------------
Can we merge SymbolTableBase with SymbolTable<ELFT> if you template this, ArchiveFile::parse and BitcodeFile::parse functions?

================
Comment at: ELF/LTO.cpp:112
@@ +111,3 @@
+  // definition in the combined LTO object will replace it when parsed.
+  auto undefine = [](Symbol *S) {
+    replaceBody<Undefined>(S, S->body()->getName(), STV_DEFAULT, 0);
----------------
undefine -> Undefine

================
Comment at: ELF/SymbolTable.cpp:136
@@ -161,2 +135,3 @@
                                                      uint8_t Visibility) {
   // Pass nullptr because absolute symbols have no corresponding input sections.
+  return cast<DefinedRegular<ELFT>>(
----------------
This comment seems obsolete.

================
Comment at: ELF/SymbolTable.cpp:266
@@ +265,3 @@
+                                      InputFile *File) {
+  auto P =
+      insert(Name, Type, StOther & 3, /*CanOmitFromDynSym*/ false,
----------------
Can you define two variables for P's members and use std::tie?

================
Comment at: ELF/SymbolTable.cpp:293
@@ +292,3 @@
+// strong defined symbols.
+static int compareDefined(std::pair<Symbol *, bool> P, uint8_t Binding) {
+  if (P.second)
----------------
Please separate the pair members so that we can give explicit name for each member.

================
Comment at: ELF/SymbolTable.cpp:330
@@ +329,3 @@
+                  /*IsUsedInRegularObj*/ true, File);
+  auto Cmp = compareDefined(P, Binding);
+  if (Cmp > 0) {
----------------
auto -> int


http://reviews.llvm.org/D19752