[PATCH] D21018: [ELF] - Basic versioned symbols support implemented.

[Rui Ueyama via llvm-commits]

ruiu added a comment.

For some reason, I received no email notifications on this review thread. George, do you have an idea why?

I haven't fully understand the core part of the patch yet, so this review is mostly on stylistic things. I'll read it again later today or tomorrow. (I'm traveling for a few days until Sunday.)


================
Comment at: ELF/OutputSections.h:246
@@ +245,3 @@
+    // Version definition flags.
+    unsigned Flags;
+    // Name hash.
----------------
Using a platform-dependent integral type for a ELF flag is a bit alarming. Can you use uint32_t?

You can format like this

  uint32_t Flags; // Version definition flags
  uint32_t Hash;  // ...

================
Comment at: ELF/OutputSections.h:254
@@ +253,3 @@
+  };
+  llvm::DenseMap<Version *, VerDefEntry> VerDefs;
+
----------------
Add a blank line.

================
Comment at: ELF/SymbolListFile.cpp:106-108
@@ +105,5 @@
+  while (!atEOF() && !Error) {
+    Config->SymbolVersions.push_back(Version());
+    Version &V = Config->SymbolVersions.back();
+    V.Name = next();
+
----------------
Does this work?

  Config->SymbolVersions.push_back({next()});

================
Comment at: ELF/SymbolListFile.cpp:111
@@ +110,3 @@
+    expect("{");
+    StringRef Peek = peek();
+    if (Peek == "global:") {
----------------
You don't need to cache the result of peek() -- it's a cheap function, and this function is not in a performance-critical path.


http://reviews.llvm.org/D21018