[PATCH] D107235: [ELF] Combine foo at v1 and foo with the same versionId if both are defined

[Peter Smith via Phabricator via llvm-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM. One quick suggestion to improve the comment and one typo in the comment.



================
Comment at: lld/ELF/Driver.cpp:2073
 
     // Check whether the default version foo@@v1 exists. If it exists, the
     // symbol can be found by the name "foo" in the symbol table.
----------------
I think this comment is now incomplete. Perhaps worth changing to something like:

For version symbol foo at v1 check the existing symbol foo.
We have two special cases to handle:
- There is a definition of foo at v1 and foo@@v1
- There is a definition of foo at v1 and foo


================
Comment at: lld/ELF/Driver.cpp:2094
+        // foo at v1 defines both foo and foo at v1. Unless foo is bound to a
+        // different version, GNU ld makes foo at v1 canonical and elimiates foo.
+        // Emulate its behavior, otherwise we would have foo at v1 and foo@@v1.
----------------
typo elimiates -> eliminates


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107235/new/

https://reviews.llvm.org/D107235