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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/SymbolTable.cpp:32
 
+static void copy(Symbol *A, Symbol *B) {
+  memcpy(A->Body.buffer, B->Body.buffer, sizeof(Symbol::Body));
----------------
grimar wrote:
> I would inline it.
I wouldn't -- I tried both and it seems this is slightly clearner.


================
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) {
----------------
grimar wrote:
> 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.
Well, I tried to use a tuple or a struct to return a value, but turned out this seems the best option I have.


================
Comment at: lld/ELF/SymbolTable.cpp:780
+    if (Ver.Name == Verstr) {
+      Version = Ver.Id;
+      return;
----------------
grimar wrote:
> 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).
I could fix it.


https://reviews.llvm.org/D34067