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

[David Blaikie via llvm-commits]

dblaikie added a subscriber: dblaikie.
dblaikie added a comment.

Just some cursory code review because I was curious to see the implementation (the aligned storage usage, etc) - nothing vital, feel free to take-or-leave as you see fit.


================
Comment at: ELF/MarkLive.cpp:141-142
@@ -141,3 +140,4 @@
+  auto MarkSymbol = [&](const SymbolBody *Sym) {
     if (Sym)
       if (auto *D = dyn_cast<DefinedRegular<ELFT>>(Sym))
         Enqueue({D->Section, D->Value});
----------------
(unrelated cleanup: this could/should just be dyn_cast_or_null)

================
Comment at: ELF/SymbolTable.cpp:282-283
@@ +281,4 @@
+      L->Type = Type;
+    } else {
+      if (auto F = L->getFile())
+        addFile(std::move(F));
----------------
Should this just be an else-if? 

================
Comment at: ELF/SymbolTable.cpp:334-335
@@ +333,4 @@
+    replaceBody<DefinedCommon>(P.first, N, Size, Alignment, StOther, Type);
+    return P.first;
+  } else if (Cmp == 0) {
+    auto *C = dyn_cast<DefinedCommon>(P.first->body());
----------------
else-after-return

================
Comment at: ELF/SymbolTable.cpp:369-373
@@ +368,7 @@
+  auto Cmp = compareDefinedNonCommon(P, Sym.getBinding());
+  if (Cmp > 0) {
+    replaceBody<DefinedRegular<ELFT>>(P.first, Name, Sym, Section);
+  } else if (Cmp == 0) {
+    reportDuplicate(P.first->body(), Section->getFile());
+  }
+  return P.first;
----------------
Seems to be a bit of inconsistency about whether single line blocks have {} or not here (& elsewhere in this change)

================
Comment at: ELF/SymbolTable.cpp:451
@@ +450,3 @@
+
+  if (P.first->body()->isUndefined()) {
+    // Weak undefined symbols should not fetch members from archives. If we were
----------------
Could invert this & use an early return to reduce indentation, perhaps.

================
Comment at: ELF/SymbolTable.cpp:489-490
@@ -365,4 +488,4 @@
   for (StringRef S : Config->Undefined)
     if (SymbolBody *Sym = find(S))
       if (auto *L = dyn_cast<Lazy>(Sym))
         if (std::unique_ptr<InputFile> File = L->getFile())
----------------
Could roll these together with dyn_cast_or_null

================
Comment at: ELF/Symbols.h:63-65
@@ +62,5 @@
+  Symbol *symbol();
+  const Symbol *symbol() const {
+    return const_cast<SymbolBody *>(this)->symbol();
+  }
+
----------------
I think, technically, from a UB perspective, you want to implement the non-const version in terms of the const version rather than the other way around.

If this object were really const then you'd have UB const_casting it and calling a member function. But if you do it the other way - you just call a const operation, knowing you have a non-const object, and cast away the constness of the result because you know it's not really const.

================
Comment at: ELF/Symbols.h:431
@@ +430,3 @@
+  SymbolBody *body() { return reinterpret_cast<SymbolBody *>(Body.buffer); }
+  const SymbolBody *body() const { return const_cast<Symbol *>(this)->body(); }
+};
----------------
Same here (wrt const/non-const overload implementation)


http://reviews.llvm.org/D19752