<div dir="ltr">Returning a const reference to a set is not wrong at all, and it involves less moving parts, no?<div><br></div><div>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.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 13, 2015 at 9:51 AM, Shankar Easwaran <span dir="ltr"><<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 2/13/2015 12:13 AM, Rui Ueyama wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, Feb 12, 2015 at 6:50 PM, Shankar Easwaran <<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>><br>
wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Rui,<br>
<br>
Unfortunately types are visible only within the class.<br>
<br>
Example :-<br>
<br>
class A {<br>
public:<br>
typedef int MyType;<br>
MyType foo();<br>
};<br>
<br>
MyType A::foo() {<br>
return 0;<br>
}<br>
<br>
int main() {<br>
A a;<br>
a.foo();<br>
}<br>
<br>
1.cpp:7:1: error: unknown type name 'MyType'; did you mean 'A::MyType'?<br>
MyType A::foo() {<br>
<br>
I wanted to only return a range than giving full access to the data<br>
structure outside the class.<br>
<br>
</blockquote>
You are not going to give a full access. I suggested returning a const<br>
reference.<br>
<br>
Actually I think returning a range for a set is bad. The whole point of<br>
using set here is because it's a set -- you want to check whether a symbol<br>
is a member of the set or not. Because you return only the iterator, you<br>
cannot call count() on it. You made O(1) to O(n).<br>
<br>
I don't think we need something tricky here. Let's just return a const<br>
reference to the field.<br>
</blockquote></div></div>
There is no usage that I need count() for, or I cant think of an usage. I want only unique entries.<span class="HOEnZb"><font color="#888888"><br>
<br>
Shankar Easwaran</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Shankar Easwaran<br>
<br>
<br>
On 2/12/2015 5:55 PM, Rui Ueyama wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, Feb 12, 2015 at 2:37 PM, Shankar Easwaran <<br>
<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>><br>
wrote:<br>
<br>
Author: shankare<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Date: Thu Feb 12 16:37:27 2015<br>
New Revision: 228968<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=228968&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=228968&view=rev</a><br>
Log:<br>
[ELF] Insert wrap symbols into a set.<br>
<br>
Symbols specified by --wrap was being inserted into a vector, change this<br>
to<br>
insert into a set, so that we have unique entries.<br>
<br>
Modified:<br>
lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFLinkingContext.cpp<br>
lld/trunk/test/elf/wrap.test<br>
<br>
Modified: lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/ReaderWriter/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/include/lld/<u></u>ReaderWriter/</a><br>
ELFLinkingContext.h?rev=<u></u>228968&r1=228967&r2=228968&<u></u>view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h (original)<br>
+++ lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h Thu Feb 12<br>
16:37:27 2015<br>
@@ -24,6 +24,7 @@<br>
#include "llvm/Support/ELF.h"<br>
#include <map><br>
#include <memory><br>
+#include <set><br>
<br>
namespace lld {<br>
class DefinedAtom;<br>
@@ -47,6 +48,7 @@ public:<br>
<br>
class ELFLinkingContext : public LinkingContext {<br>
public:<br>
+ typedef std::set<StringRef>::iterator StringRefSetIterT;<br>
<br>
You defined StringSetIterT and<br>
</blockquote>
<br>
/// \brief The type of ELF executable that the linker<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
/// creates.<br>
@@ -305,7 +307,7 @@ public:<br>
// --wrap option.<br>
void addWrapForSymbol(StringRef);<br>
<br>
- StringRefVector wrapCalls() const;<br>
+ range<std::set<StringRef>::<u></u>iterator> wrapCalls() const;<br>
<br>
private:<br>
ELFLinkingContext() LLVM_DELETED_FUNCTION;<br>
@@ -346,7 +348,7 @@ protected:<br>
StringRef _soname;<br>
StringRefVector _rpathList;<br>
StringRefVector _rpathLinkList;<br>
- StringRefVector _wrapCalls;<br>
+ std::set<StringRef> _wrapCalls;<br>
std::map<std::string, uint64_t> _absoluteSymbols;<br>
llvm::StringSet<> _dynamicallyExportedSymbols;<br>
std::vector<std::unique_ptr<<u></u>script::Parser>> _scripts;<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFLinkingContext.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/lib/<u></u>ReaderWriter/ELF/</a><br>
ELFLinkingContext.cpp?rev=<u></u>228968&r1=228967&r2=228968&<u></u>view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFLinkingContext.cpp (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFLinkingContext.cpp Thu Feb 12<br>
16:37:27 2015<br>
@@ -258,10 +258,11 @@ std::string ELFLinkingContext::demangle(<br>
<br>
// Support --wrap option.<br>
void ELFLinkingContext::<u></u>addWrapForSymbol(StringRef symbol) {<br>
- _wrapCalls.push_back(symbol);<br>
+ _wrapCalls.insert(symbol);<br>
}<br>
<br>
-ELFLinkingContext::<u></u>StringRefVector ELFLinkingContext::wrapCalls()<br>
const {<br>
+range<ELFLinkingContext::<u></u>StringRefSetIterT><br>
<br>
use that here with range<>. Is this needed? Why don't you just return<br>
</blockquote>
const<br>
std::set<StringRef> & from this function?<br>
<br>
+ELFLinkingContext::wrapCalls(<u></u>) const {<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
return _wrapCalls;<br>
}<br>
<br>
<br>
Modified: lld/trunk/test/elf/wrap.test<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/test/elf/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/test/elf/</a><br>
wrap.test?rev=228968&r1=<u></u>228967&r2=228968&view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/test/elf/wrap.test (original)<br>
+++ lld/trunk/test/elf/wrap.test Thu Feb 12 16:37:27 2015<br>
@@ -26,9 +26,9 @@<br>
#RUN: yaml2obj -format=elf -docnum 2 %s -o %t.wrapfoo.o<br>
#RUN: yaml2obj -format=elf -docnum 3 %s -o %t.realfoo.o<br>
#RUN: lld -flavor gnu -target x86_64 %t.main.o %t.wrapfoo.o<br>
%t.realfoo.o \<br>
-#RUN: --wrap foo --noinhibit-exec --output-filetype=yaml -o %t2.out<br>
+#RUN: --wrap foo --wrap foo --noinhibit-exec --output-filetype=yaml -o<br>
%t2.out<br>
#RUN: lld -flavor gnu -target x86_64 %t.main.o %t.wrapfoo.o \<br>
-#RUN: --wrap foo --noinhibit-exec --output-filetype=yaml -o<br>
%t2.out.undef<br>
2>&1 | \<br>
+#RUN: --wrap foo --wrap foo --noinhibit-exec --output-filetype=yaml -o<br>
%t2.out.undef 2>&1 | \<br>
#RUN: FileCheck %s -check-prefix=CHECKUNDEF<br>
#CHECKWRAP: - name: main<br>
#CHECKWRAP: references:<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
<br>
</blockquote></blockquote>
--<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted<br>
by the Linux Foundation<br>
<br>
<br>
</blockquote></blockquote>
<br>
<br>
-- <br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation<br>
<br>
</div></div></blockquote></div><br></div>