[lld] r228968 - [ELF] Insert wrap symbols into a set.
Rui Ueyama
ruiu at google.com
Fri Feb 13 11:30:19 PST 2015
Returning a const reference to a set is not wrong at all, and it involves
less moving parts, no?
The other reason why I prefer a set over range here is because I don't want
to see more uses of range in LLD at this moment. I believe range is
designed after N3350, which didn't make it to the standard yet. It doesn't
feel great that this generic library lives under LLD. If we really need
this one, we should move the file to LLVM support directory. Otherwise, I
don't like to build a local rule to use range in LLD. After all other LLVM
projects are not using this range.
On Fri, Feb 13, 2015 at 9:51 AM, Shankar Easwaran <shankare at codeaurora.org>
wrote:
> On 2/13/2015 12:13 AM, Rui Ueyama wrote:
>
>> On Thu, Feb 12, 2015 at 6:50 PM, Shankar Easwaran <
>> shankare at codeaurora.org>
>> wrote:
>>
>> Hi Rui,
>>>
>>> Unfortunately types are visible only within the class.
>>>
>>> Example :-
>>>
>>> class A {
>>> public:
>>> typedef int MyType;
>>> MyType foo();
>>> };
>>>
>>> MyType A::foo() {
>>> return 0;
>>> }
>>>
>>> int main() {
>>> A a;
>>> a.foo();
>>> }
>>>
>>> 1.cpp:7:1: error: unknown type name 'MyType'; did you mean 'A::MyType'?
>>> MyType A::foo() {
>>>
>>> I wanted to only return a range than giving full access to the data
>>> structure outside the class.
>>>
>>> You are not going to give a full access. I suggested returning a const
>> reference.
>>
>> Actually I think returning a range for a set is bad. The whole point of
>> using set here is because it's a set -- you want to check whether a symbol
>> is a member of the set or not. Because you return only the iterator, you
>> cannot call count() on it. You made O(1) to O(n).
>>
>> I don't think we need something tricky here. Let's just return a const
>> reference to the field.
>>
> There is no usage that I need count() for, or I cant think of an usage. I
> want only unique entries.
>
> Shankar Easwaran
>
>
>
>> Shankar Easwaran
>>>
>>>
>>> On 2/12/2015 5:55 PM, Rui Ueyama wrote:
>>>
>>> On Thu, Feb 12, 2015 at 2:37 PM, Shankar Easwaran <
>>>> shankare at codeaurora.org>
>>>> wrote:
>>>>
>>>> Author: shankare
>>>>
>>>>> Date: Thu Feb 12 16:37:27 2015
>>>>> New Revision: 228968
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=228968&view=rev
>>>>> Log:
>>>>> [ELF] Insert wrap symbols into a set.
>>>>>
>>>>> Symbols specified by --wrap was being inserted into a vector, change
>>>>> this
>>>>> to
>>>>> insert into a set, so that we have unique entries.
>>>>>
>>>>> Modified:
>>>>> lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h
>>>>> lld/trunk/lib/ReaderWriter/ELF/ELFLinkingContext.cpp
>>>>> lld/trunk/test/elf/wrap.test
>>>>>
>>>>> Modified: lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/
>>>>> ReaderWriter/
>>>>> ELFLinkingContext.h?rev=228968&r1=228967&r2=228968&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h (original)
>>>>> +++ lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h Thu Feb 12
>>>>> 16:37:27 2015
>>>>> @@ -24,6 +24,7 @@
>>>>> #include "llvm/Support/ELF.h"
>>>>> #include <map>
>>>>> #include <memory>
>>>>> +#include <set>
>>>>>
>>>>> namespace lld {
>>>>> class DefinedAtom;
>>>>> @@ -47,6 +48,7 @@ public:
>>>>>
>>>>> class ELFLinkingContext : public LinkingContext {
>>>>> public:
>>>>> + typedef std::set<StringRef>::iterator StringRefSetIterT;
>>>>>
>>>>> You defined StringSetIterT and
>>>>>
>>>>
>>>> /// \brief The type of ELF executable that the linker
>>>>
>>>>> /// creates.
>>>>> @@ -305,7 +307,7 @@ public:
>>>>> // --wrap option.
>>>>> void addWrapForSymbol(StringRef);
>>>>>
>>>>> - StringRefVector wrapCalls() const;
>>>>> + range<std::set<StringRef>::iterator> wrapCalls() const;
>>>>>
>>>>> private:
>>>>> ELFLinkingContext() LLVM_DELETED_FUNCTION;
>>>>> @@ -346,7 +348,7 @@ protected:
>>>>> StringRef _soname;
>>>>> StringRefVector _rpathList;
>>>>> StringRefVector _rpathLinkList;
>>>>> - StringRefVector _wrapCalls;
>>>>> + std::set<StringRef> _wrapCalls;
>>>>> std::map<std::string, uint64_t> _absoluteSymbols;
>>>>> llvm::StringSet<> _dynamicallyExportedSymbols;
>>>>> std::vector<std::unique_ptr<script::Parser>> _scripts;
>>>>>
>>>>> Modified: lld/trunk/lib/ReaderWriter/ELF/ELFLinkingContext.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/
>>>>> ELFLinkingContext.cpp?rev=228968&r1=228967&r2=228968&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/lib/ReaderWriter/ELF/ELFLinkingContext.cpp (original)
>>>>> +++ lld/trunk/lib/ReaderWriter/ELF/ELFLinkingContext.cpp Thu Feb 12
>>>>> 16:37:27 2015
>>>>> @@ -258,10 +258,11 @@ std::string ELFLinkingContext::demangle(
>>>>>
>>>>> // Support --wrap option.
>>>>> void ELFLinkingContext::addWrapForSymbol(StringRef symbol) {
>>>>> - _wrapCalls.push_back(symbol);
>>>>> + _wrapCalls.insert(symbol);
>>>>> }
>>>>>
>>>>> -ELFLinkingContext::StringRefVector ELFLinkingContext::wrapCalls()
>>>>> const {
>>>>> +range<ELFLinkingContext::StringRefSetIterT>
>>>>>
>>>>> use that here with range<>. Is this needed? Why don't you just return
>>>>>
>>>> const
>>>> std::set<StringRef> & from this function?
>>>>
>>>> +ELFLinkingContext::wrapCalls() const {
>>>>
>>>> return _wrapCalls;
>>>>> }
>>>>>
>>>>>
>>>>> Modified: lld/trunk/test/elf/wrap.test
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/test/elf/
>>>>> wrap.test?rev=228968&r1=228967&r2=228968&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/test/elf/wrap.test (original)
>>>>> +++ lld/trunk/test/elf/wrap.test Thu Feb 12 16:37:27 2015
>>>>> @@ -26,9 +26,9 @@
>>>>> #RUN: yaml2obj -format=elf -docnum 2 %s -o %t.wrapfoo.o
>>>>> #RUN: yaml2obj -format=elf -docnum 3 %s -o %t.realfoo.o
>>>>> #RUN: lld -flavor gnu -target x86_64 %t.main.o %t.wrapfoo.o
>>>>> %t.realfoo.o \
>>>>> -#RUN: --wrap foo --noinhibit-exec --output-filetype=yaml -o %t2.out
>>>>> +#RUN: --wrap foo --wrap foo --noinhibit-exec --output-filetype=yaml -o
>>>>> %t2.out
>>>>> #RUN: lld -flavor gnu -target x86_64 %t.main.o %t.wrapfoo.o \
>>>>> -#RUN: --wrap foo --noinhibit-exec --output-filetype=yaml -o
>>>>> %t2.out.undef
>>>>> 2>&1 | \
>>>>> +#RUN: --wrap foo --wrap foo --noinhibit-exec --output-filetype=yaml -o
>>>>> %t2.out.undef 2>&1 | \
>>>>> #RUN: FileCheck %s -check-prefix=CHECKUNDEF
>>>>> #CHECKWRAP: - name: main
>>>>> #CHECKWRAP: references:
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>> --
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
>>> by the Linux Foundation
>>>
>>>
>>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by the Linux Foundation
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150213/3d43aa2e/attachment.html>
More information about the llvm-commits
mailing list