[PATCH] D34067: Use symbols with the default version to resolve unversioned symbols.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Thanks for taking a look on that ! Really.

Honestly I think https://reviews.llvm.org/D33680 is more clear for my eye. Let me do one more update for it,
I have one change in mind I did not include ealier, we can discuss which way fits better after that.

My conserns about this patch are inline.



================
Comment at: lld/ELF/SymbolTable.cpp:32
 
+static void copy(Symbol *A, Symbol *B) {
+  memcpy(A->Body.buffer, B->Body.buffer, sizeof(Symbol::Body));
----------------
I would inline it.


================
Comment at: lld/ELF/SymbolTable.cpp:755
+// <name>@<version> or <name>@@<version>.
+static void parseSymbolVersion(const SymbolBody &B, uint16_t &Version,
+                               bool &IsDefault, StringRef &NewName) {
----------------
So you are mutating arguments here. Generally you are asking to avoid that in reviews. 
And I think I agree with that strategy. 
Mutating arguments really made me be confusing, please see next comment.


================
Comment at: lld/ELF/SymbolTable.cpp:780
+    if (Ver.Name == Verstr) {
+      Version = Ver.Id;
+      return;
----------------
So Version can have valid 0 value it seems ?

But below you do check for 0:

```
    if (Ver != 0)
      Sym->VersionId = IsDefault ? Ver : (Ver | VERSYM_HIDDEN);
```

That become visible and confusing for me mostly because of mutating arguments.
(I was a bit lost in what this method do in general).


https://reviews.llvm.org/D34067