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

[George Rimar via llvm-commits]

grimar added a comment.

In http://reviews.llvm.org/D21018#452308, @ruiu wrote:

> Here is my first round of review.


Thanks for review ! My response to comments are below.


================
Comment at: ELF/InputFiles.cpp:343
@@ -341,1 +342,3 @@
 
+static StringRef filterVersion(StringRef Name) {
+  StringRef Version;
----------------
ruiu wrote:
> Why do you need this?
Right, for this patch and its trivial versionscript and cases this is excessive. That left from my experiments with bit more complex cases.
Removed.

================
Comment at: ELF/SymbolListFile.cpp:84
@@ -79,2 +83,3 @@
 
 void VersionScriptParser::run() {
+  bool HasAnonymousTag = false;
----------------
ruiu wrote:
> A version definition without a version name is not actually a version definition in the sense that they controls only visibility and have nothing to do with symbol versioning. I think it is better to handle that case completely differently from symbol versioning.
Ok. I was thinking about that as about a some virtual "base version of the library".

Spec says: "Symbols defined in the library which aren't specifically bound to a version node are effectively bound to an unspecified base version of the library. " (http://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_25.html). That is not exactly our case probably, but it is sounds relative.
I reimplemented code according to your comment though, lets see how it goes.

================
Comment at: ELF/SymbolListFile.cpp:121
@@ +120,3 @@
+    if (peek() != ";")
+      V.Parent = next();
+    expect(";");
----------------
ruiu wrote:
> Since you are not using this member at all, please remove Parent member from the struct and just discard the next token here.
Done.

================
Comment at: ELF/SymbolListFile.h:23-27
@@ -21,1 +22,7 @@
 
+struct Version {
+  StringRef Name;
+  StringRef Parent;
+  std::vector<llvm::StringRef> Globals;
+};
+
----------------
ruiu wrote:
> Move this definition to Config.h and make version definitions a member of Config class.
Done.


http://reviews.llvm.org/D21018