[PATCH] D19430: ELF: Implement basic support for --version-script.

[Rui Ueyama via llvm-commits]

ruiu added a comment.

LGTM. This is looking nice. A few minor comments below.


================
Comment at: ELF/Config.h:15
@@ -14,2 +14,3 @@
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/ELF.h"
----------------
You are not using this.

================
Comment at: ELF/Driver.cpp:149-157
@@ -148,6 +148,11 @@
 
 void LinkerDriver::readDynamicList(StringRef Path) {
   if (Optional<MemoryBufferRef> Buffer = readFile(Path))
     parseDynamicList(*Buffer);
 }
 
+void LinkerDriver::readVersionScript(StringRef Path) {
+  if (Optional<MemoryBufferRef> Buffer = readFile(Path))
+    parseVersionScript(*Buffer);
+}
+
----------------
I'd inline these functions.

================
Comment at: ELF/SymbolListFile.cpp:83-95
@@ +82,15 @@
+  expect("{");
+  if (peek() == "global:") {
+    next();
+    while (!Error) {
+      Config->VersionScriptGlobals.push_back(next());
+      expect(";");
+      if (peek() == "local:") {
+        next();
+        break;
+      }
+    }
+  } else {
+    expect("local:");
+  }
+  expect("*");
----------------
I think this is slightly more readable.

  if (peek() == "global:") {
    next();
    while (!Error) {
      Config->VersionScriptGlobals.push_back(next());
      expect(";");
      if (peek() == "local:")
        break;
    }
  }
  expect("local:");



http://reviews.llvm.org/D19430