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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.h:246
@@ +245,3 @@
+    // Version definition flags.
+    unsigned Flags;
+    // Name hash.
----------------
ruiu wrote:
> 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;  // ...
Done.

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

================
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();
+
----------------
ruiu wrote:
> Does this work?
> 
>   Config->SymbolVersions.push_back({next()});
Actually this does not. Though it should be valid С++ construction and works in msvs/gcc,
as far I know clang produces an error if you do not specify all members in aggregate specialization.
So this might be:

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

to work. And I do not like this, because if you change the members of struct (amount of them),
you have to be sure to check and update all such places to support clang.

I updated this place, since it is single, but above was the reason why I did not do that initially (even was thinking about adding constructor for Name to Version struct).

================
Comment at: ELF/SymbolListFile.cpp:111
@@ +110,3 @@
+    expect("{");
+    StringRef Peek = peek();
+    if (Peek == "global:") {
----------------
ruiu wrote:
> 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.
Done.


http://reviews.llvm.org/D21018