<div dir="ltr">As per IRC conversation, addressed and then some in r195260 & r195261. Thanks for the review.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Nov 20, 2013 at 8:34 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">By the looks of it, if I removed the move operations these tests would still pass, right? (as they do in C++98 mode)<br>
<br>Since move semantics are an optimization I suppose this is sort of to be expected, though the tests could <br>
<div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">On Wed, Nov 20, 2013 at 3:14 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: chandlerc<br>
Date: Wed Nov 20 05:14:33 2013<br>
New Revision: 195239<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=195239&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=195239&view=rev</a><br>
Log:<br>
Give SmallPtrSet move semantics when we have R-value references.<br>
Somehow, this ADT got missed which is moderately terrifying considering<br>
the efficiency of move for it.<br>
<br>
The code to implement move semantics for it is pretty horrible<br>
currently but was written to reasonably closely match the rest of the<br>
code. Unittests that cover both copying and moving (at a basic level)<br>
added.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ADT/SmallPtrSet.h<br>
llvm/trunk/lib/Support/SmallPtrSet.cpp<br>
llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/SmallPtrSet.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallPtrSet.h?rev=195239&r1=195238&r2=195239&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallPtrSet.h?rev=195239&r1=195238&r2=195239&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ADT/SmallPtrSet.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/SmallPtrSet.h Wed Nov 20 05:14:33 2013<br>
@@ -60,8 +60,12 @@ protected:<br>
unsigned NumElements;<br>
unsigned NumTombstones;<br>
<br>
- // Helper to copy construct a SmallPtrSet.<br>
- SmallPtrSetImpl(const void **SmallStorage, const SmallPtrSetImpl& that);<br>
+ // Helpers to copy and move construct a SmallPtrSet.<br>
+ SmallPtrSetImpl(const void **SmallStorage, const SmallPtrSetImpl &that);<br>
+#if LLVM_HAS_RVALUE_REFERENCES<br>
+ SmallPtrSetImpl(const void **SmallStorage, unsigned SmallSize,<br>
+ SmallPtrSetImpl &&that);<br>
+#endif<br>
explicit SmallPtrSetImpl(const void **SmallStorage, unsigned SmallSize) :<br>
SmallArray(SmallStorage), CurArray(SmallStorage), CurArraySize(SmallSize) {<br>
assert(SmallSize && (SmallSize & (SmallSize-1)) == 0 &&<br>
@@ -135,6 +139,9 @@ protected:<br>
void swap(SmallPtrSetImpl &RHS);<br>
<br>
void CopyFrom(const SmallPtrSetImpl &RHS);<br>
+#if LLVM_HAS_RVALUE_REFERENCES<br>
+ void MoveFrom(SmallPtrSetImpl &&RHS);<br>
+#endif<br>
};<br>
<br>
/// SmallPtrSetIteratorImpl - This is the common base class shared between all<br>
@@ -242,6 +249,10 @@ class SmallPtrSet : public SmallPtrSetIm<br>
public:<br>
SmallPtrSet() : SmallPtrSetImpl(SmallStorage, SmallSizePowTwo) {}<br>
SmallPtrSet(const SmallPtrSet &that) : SmallPtrSetImpl(SmallStorage, that) {}<br>
+#if LLVM_HAS_RVALUE_REFERENCES<br>
+ SmallPtrSet(SmallPtrSet &&that)<br>
+ : SmallPtrSetImpl(SmallStorage, SmallSizePowTwo, std::move(that)) {}<br>
+#endif<br>
<br>
template<typename It><br>
SmallPtrSet(It I, It E) : SmallPtrSetImpl(SmallStorage, SmallSizePowTwo) {<br>
@@ -280,14 +291,20 @@ public:<br>
return iterator(CurArray+CurArraySize, CurArray+CurArraySize);<br>
}<br>
<br>
- // Allow assignment from any smallptrset with the same element type even if it<br>
- // doesn't have the same smallsize.<br>
- const SmallPtrSet<PtrType, SmallSize>&<br>
+ SmallPtrSet<PtrType, SmallSize> &<br></blockquote><div><br></div></div></div><div>We could have a test for this ((a = b).insert(...) for example).</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
operator=(const SmallPtrSet<PtrType, SmallSize> &RHS) {<br>
CopyFrom(RHS);<br>
return *this;<br>
}<br>
<br>
+#if LLVM_HAS_RVALUE_REFERENCES<br>
+ SmallPtrSet<PtrType, SmallSize>&<br>
+ operator=(SmallPtrSet<PtrType, SmallSize> &&RHS) {<br>
+ MoveFrom(std::move(RHS));<br>
+ return *this;<br>
+ }<br>
+#endif<br>
+<br>
/// swap - Swaps the elements of two sets.<br>
void swap(SmallPtrSet<PtrType, SmallSize> &RHS) {<br>
SmallPtrSetImpl::swap(RHS);<br>
<br>
Modified: llvm/trunk/lib/Support/SmallPtrSet.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/SmallPtrSet.cpp?rev=195239&r1=195238&r2=195239&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/SmallPtrSet.cpp?rev=195239&r1=195238&r2=195239&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/SmallPtrSet.cpp (original)<br>
+++ llvm/trunk/lib/Support/SmallPtrSet.cpp Wed Nov 20 05:14:33 2013<br>
@@ -186,6 +186,29 @@ SmallPtrSetImpl::SmallPtrSetImpl(const v<br>
NumTombstones = that.NumTombstones;<br>
}<br>
<br>
+#if LLVM_HAS_RVALUE_REFERENCES<br>
+SmallPtrSetImpl::SmallPtrSetImpl(const void **SmallStorage, unsigned SmallSize,<br>
+ SmallPtrSetImpl &&that) {<br>
+ SmallArray = SmallStorage;<br>
+<br>
+ // Copy over the basic members.<br>
+ CurArraySize = that.CurArraySize;<br>
+ NumElements = that.NumElements;<br>
+ NumTombstones = that.NumTombstones;<br>
+<br>
+ // When small, just copy into our small buffer.<br>
+ if (that.isSmall()) {<br>
+ CurArray = SmallArray;<br>
+ memcpy(CurArray, that.CurArray, sizeof(void *) * CurArraySize);<br>
+ return;<br>
+ }<br>
+<br>
+ // Otherwise, we steal the large memory allocation and no copy is needed.<br>
+ CurArray = that.CurArray;<br>
+ that.CurArray = that.SmallArray;<br></blockquote><div><br></div></div></div><div>I haven't looked at this closely yet, but do you need to reset that.CurArraySize/NumElements/NumTombstones or anything else? You have no test coverage on the moved-from objects, what guarantees, if any, are provided? (empty? valid-but-unspecified? invalid-but-destructible and move-assignable-to?)</div>
<div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+#endif<br>
+<br>
/// CopyFrom - implement operator= from a smallptrset that has the same pointer<br>
/// type, but may have a different small size.<br>
void SmallPtrSetImpl::CopyFrom(const SmallPtrSetImpl &RHS) {<br>
@@ -222,6 +245,27 @@ void SmallPtrSetImpl::CopyFrom(const Sma<br>
NumTombstones = RHS.NumTombstones;<br>
}<br>
<br>
+#if LLVM_HAS_RVALUE_REFERENCES<br>
+void SmallPtrSetImpl::MoveFrom(SmallPtrSetImpl &&RHS) {<br>
+ if (!isSmall())<br>
+ free(CurArray);<br>
+<br>
+ if (RHS.isSmall()) {<br>
+ // Copy a small RHS rather than moving.<br>
+ CurArray = SmallArray;<br>
+ memcpy(CurArray, RHS.CurArray, sizeof(void*)*RHS.CurArraySize);<br>
+ } else {<br>
+ CurArray = RHS.CurArray;<br>
+ RHS.CurArray = RHS.SmallArray;<br>
+ }<br>
+<br>
+ // Copy the rest of the trivial members.<br>
+ CurArraySize = RHS.CurArraySize;<br>
+ NumElements = RHS.NumElements;<br>
+ NumTombstones = RHS.NumTombstones;<br></blockquote><div><br></div></div></div><div>Similarly here, I'm wondering what state the moved-from object ends up in.<br><br>I'm guessing the RHS.CurArray = RHS.SmallArray is enough to make the object invalid-but-destructible (& move-assignable to?) and that's all you want to guarantee?</div>
<div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+#endif<br>
+<br>
void SmallPtrSetImpl::swap(SmallPtrSetImpl &RHS) {<br>
if (this == &RHS) return;<br>
<br>
<br>
Modified: llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp?rev=195239&r1=195238&r2=195239&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp?rev=195239&r1=195238&r2=195239&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp (original)<br>
+++ llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp Wed Nov 20 05:14:33 2013<br>
@@ -71,6 +71,55 @@ TEST(SmallPtrSetTest, GrowthTest) {<br>
EXPECT_EQ(1,buf[i]);<br>
}<br>
<br>
+TEST(SmallPtrSetTest, CopyAndMoveTest) {<br>
+ int buf[8];<br>
+ for (int i = 0; i < 8; ++i)<br>
+ buf[i] = 0;<br>
+<br>
+ SmallPtrSet<int *, 4> s1;<br>
+ s1.insert(&buf[0]);<br>
+ s1.insert(&buf[1]);<br>
+ s1.insert(&buf[2]);<br>
+ s1.insert(&buf[3]);<br>
+ EXPECT_EQ(4U, s1.size());<br>
+ for (int i = 0; i < 8; ++i)<br>
+ if (i < 4)<br>
+ EXPECT_TRUE(s1.count(&buf[i]));<br>
+ else<br>
+ EXPECT_FALSE(s1.count(&buf[i]));<br>
+<br>
+ SmallPtrSet<int *, 4> s2(s1);<br>
+ EXPECT_EQ(4U, s2.size());<br>
+ for (int i = 0; i < 8; ++i)<br>
+ if (i < 4)<br>
+ EXPECT_TRUE(s2.count(&buf[i]));<br>
+ else<br>
+ EXPECT_FALSE(s2.count(&buf[i]));<br>
+<br>
+ s1 = s2;<br>
+ EXPECT_EQ(4U, s1.size());<br>
+ for (int i = 0; i < 8; ++i)<br>
+ if (i < 4)<br>
+ EXPECT_TRUE(s1.count(&buf[i]));<br>
+ else<br>
+ EXPECT_FALSE(s1.count(&buf[i]));<br>
+<br>
+ SmallPtrSet<int *, 4> s3(llvm_move(s1));<br>
+ EXPECT_EQ(4U, s3.size());<br>
+ for (int i = 0; i < 8; ++i)<br>
+ if (i < 4)<br>
+ EXPECT_TRUE(s3.count(&buf[i]));<br>
+ else<br>
+ EXPECT_FALSE(s3.count(&buf[i]));<br>
+<br>
+ s1 = llvm_move(s3);<br>
+ EXPECT_EQ(4U, s1.size());<br>
+ for (int i = 0; i < 8; ++i)<br>
+ if (i < 4)<br>
+ EXPECT_TRUE(s1.count(&buf[i]));<br>
+ else<br>
+ EXPECT_FALSE(s1.count(&buf[i]));<br>
+}<br>
<br>
TEST(SmallPtrSetTest, SwapTest) {<br>
int buf[10];<br>
<br>
<br>
_______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div><br></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>