[lld] r335337 - [ELF] - Change how we handle suplicate -wrap. [NFC]

[Shoaib Meenai via llvm-commits]

Sorry, I'd missed that this was an NFC cleanup on top of an already committed patch (which added the test case).

I think the new code is definitely better, in that the purpose of the code is to prevent duplicate -wrap entries, and the previous code was doing this via a sort + uniquify, which is indirect and makes you think that the sorted order is significant (which is the point George had raised during review). I just think it would be more straightforward to express as a loop rather than a find_if.

From: Rui Ueyama <ruiu at google.com>
Date: Sunday, June 24, 2018 at 10:45 PM
To: Shoaib Meenai <smeenai at fb.com>
Cc: George Rimar <grimar at accesssoftek.com>, llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [lld] r335337 - [ELF] - Change how we handle suplicate -wrap. [NFC]

I think I agree with Shoaib about this patch. The old code doesn't look sophisticated as the new code which uses a higher-order function, but the old code was more aligned with the lld's style. In lld, we prefer plain for-loop over a higher-order function. Could you please revert?

On Sat, Jun 23, 2018 at 5:19 AM Shoaib Meenai via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
Test case?

I think a loop might be cleaner than the find_if here, and it would be more consistent with LLD's general style.

From: llvm-commits <llvm-commits-bounces at lists.llvm.org<mailto:llvm-commits-bounces at lists.llvm.org>> on behalf of George Rimar via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>>
Reply-To: George Rimar <grimar at accesssoftek.com<mailto:grimar at accesssoftek.com>>
Date: Friday, June 22, 2018 at 4:22 AM
To: "llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>" <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>>
Subject: [lld] r335337 - [ELF] - Change how we handle suplicate -wrap. [NFC]

Author: grimar
Date: Fri Jun 22 04:18:11 2018
New Revision: 335337

URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D335337-26view-3Drev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=kLg3NLMsMNgV7J0ezhc_ofICBgptUy3l1QaDZyvKYdQ&s=nNHBpytUwWlQ1IdJyvIOPV8tCx6kVtvJq1lhPiUMmMw&e=
Log:
[ELF] - Change how we handle suplicate -wrap. [NFC]

This avoids doing llvm::sort and std::unique for -wrap options.
I think it is more clean way.

Modified:
    lld/trunk/ELF/Driver.cpp
    lld/trunk/ELF/SymbolTable.cpp

Modified: lld/trunk/ELF/Driver.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lld_trunk_ELF_Driver.cpp-3Frev-3D335337-26r1-3D335336-26r2-3D335337-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=kLg3NLMsMNgV7J0ezhc_ofICBgptUy3l1QaDZyvKYdQ&s=85yxVCAOkTRpBEXBvcR0HT0kuimCRlTD70Nd5anqlgM&e=
==============================================================================
--- lld/trunk/ELF/Driver.cpp (original)
+++ lld/trunk/ELF/Driver.cpp Fri Jun 22 04:18:11 2018
@@ -1304,11 +1304,8 @@ template <class ELFT> void LinkerDriver:
     Symtab->scanVersionScript();
   // Create wrapped symbols for -wrap option.
-  std::vector<std::string> Wraps = Args.getAllArgValues(OPT_wrap);
-  llvm::sort(Wraps.begin(), Wraps.end());
-  Wraps.erase(std::unique(Wraps.begin(), Wraps.end()), Wraps.end());
-  for (StringRef Name : Wraps)
-    Symtab->addSymbolWrap<ELFT>(Name);
+  for (auto *Arg : Args.filtered(OPT_wrap))
+    Symtab->addSymbolWrap<ELFT>(Arg->getValue());
   // Do link-time optimization if given files are LLVM bitcode files.
   // This compiles bitcode files into real object files.

Modified: lld/trunk/ELF/SymbolTable.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lld_trunk_ELF_SymbolTable.cpp-3Frev-3D335337-26r1-3D335336-26r2-3D335337-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=kLg3NLMsMNgV7J0ezhc_ofICBgptUy3l1QaDZyvKYdQ&s=0MyoZRpNw0irOfUP_nOvDilUeOFR3HUQKhRfGzRTWY4&e=
==============================================================================
--- lld/trunk/ELF/SymbolTable.cpp (original)
+++ lld/trunk/ELF/SymbolTable.cpp Fri Jun 22 04:18:11 2018
@@ -158,6 +158,13 @@ template <class ELFT> void SymbolTable::
   Symbol *Sym = find(Name);
   if (!Sym)
     return;
+
+  // Do not wrap the same symbol twice.
+  if (llvm::find_if(WrappedSymbols, [&](const WrappedSymbol &S) {
+        return S.Sym == Sym;
+      }) != WrappedSymbols.end())
+    return;
+
   Symbol *Real = addUndefined<ELFT>(Saver.save("__real_" + Name));
   Symbol *Wrap = addUndefined<ELFT>(Saver.save("__wrap_" + Name));
   WrappedSymbols.push_back({Sym, Real, Wrap});


_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=kLg3NLMsMNgV7J0ezhc_ofICBgptUy3l1QaDZyvKYdQ&s=x-WhvYaGLgFgMwtmeFgkgjIgvFXFgPwlbSAIN45lIWk&e=

_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=nqnk3lxjrc-mF9WAxBuhNqDfwkHIal5ZSwy_ZFBVpV4&s=ni3Hw-IrgLoyuL9nYfPHRMjEq_ISMK7M_2ehqwsAgAE&e=>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180625/0faa494d/attachment.html>