[PATCH] D21556: [ELF] - Implemented version script hierarchies.

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits.


================
Comment at: ELF/Config.h:39
@@ -38,2 +38,3 @@
   Version(llvm::StringRef Name) : Name(Name) {}
+  llvm::StringRef Parent;
   llvm::StringRef Name;
----------------
Yup, I think this is a better name. I'd move this line after `Name` since `Name` is more important.

================
Comment at: ELF/OutputSections.cpp:1510
@@ +1509,3 @@
+
+  // Make previous definition name be correctly connected to this one.
+  Verdaux[-1].vda_next = sizeof(Elf_Verdaux);
----------------
I don't think this comment helps reader to understand this piece of code. Instead, we could avoid [-1] by doing this, but I'd think your code is easier to read. So I'd just do without comment.

  Verdaux->vda_name = StrTabOffset;
  if (HasParent) {
    Verdaux->vda_next = sizeof(Elf_Verdaux);
    ++Verdaux;
    Verdaux->vda_name = getVersionNameStrTabOffset(ParentName);
    Verdaux->vda_next = 0;
  } else {
    Verdaux->vda_next = 0;
  }
  ++Verdaux;

One thing I would do is to not do early return in this function. I believe this is easier to read in this case.

================
Comment at: ELF/OutputSections.cpp:1524
@@ -1500,3 +1523,3 @@
   writeDefinition(Verdef, Verdaux, VER_FLG_BASE, 1, getFileDefName(),
-                  FileDefNameOff);
+                  FileDefNameOff, "" /* Dependency */);
 
----------------
Update comment.


http://reviews.llvm.org/D21556