[llvm] r226899 - [ADT] Add move operations to SmallVector<T, N> from SmallVectorImpl<T>.

[Pete Cooper]

I think it's useful for an optimization but also necessary that it's not in the assignment operator.

For the constructor doing this check just avoids needlessly settings fields to be empty when we know they are already empty.

For the assignment operator, we don't know that the 'this' vector is empty or not so we have to copy rhs over it, otherwise we'd fail to clear out vector when rhs is empty.

Pete

Sent from my iPhone

> On Jan 23, 2015, at 10:57 AM, Lang Hames <lhames at gmail.com> wrote:
> 
> Hi Dave,
> 
> I didn't delve into the code to check,  I just cribbed from the existing implementations. My inclination is that that check should be pushed down into SmallVectorImpl's methods though.
> 
> - Lang.
> 
>> On Fri, Jan 23, 2015 at 12:02 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>>> On Thu, Jan 22, 2015 at 10:25 PM, Lang Hames <lhames at gmail.com> wrote:
>>> Author: lhames
>>> Date: Fri Jan 23 00:25:17 2015
>>> New Revision: 226899
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=226899&view=rev
>>> Log:
>>> [ADT] Add move operations to SmallVector<T,N> from SmallVectorImpl<T>.
>>> 
>>> This makes it possible to move between SmallVectors of different sizes.
>>> 
>>> Thanks to Dave Blaikie and Duncan Smith for patch feedback.
>>> 
>>> Modified:
>>>     llvm/trunk/include/llvm/ADT/SmallVector.h
>>>     llvm/trunk/unittests/ADT/SmallVectorTest.cpp
>>> 
>>> Modified: llvm/trunk/include/llvm/ADT/SmallVector.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallVector.h?rev=226899&r1=226898&r2=226899&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/SmallVector.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/SmallVector.h Fri Jan 23 00:25:17 2015
>>> @@ -921,6 +921,17 @@ public:
>>>      SmallVectorImpl<T>::operator=(::std::move(RHS));
>>>      return *this;
>>>    }
>>> +
>>> +  SmallVector(SmallVectorImpl<T> &&RHS) : SmallVectorImpl<T>(N) {
>>> +    if (!RHS.empty())
>> 
>> Is this ^ check necessary/useful for some reason? (why in the ctor, but not in the assignment operator?)
>>  
>>> +      SmallVectorImpl<T>::operator=(::std::move(RHS));
>>> +  }
>>> +
>>> +  const SmallVector &operator=(SmallVectorImpl<T> &&RHS) {
>>> +    SmallVectorImpl<T>::operator=(::std::move(RHS));
>>> +    return *this;
>>> +  }
>>> +
>>>  };
>>> 
>>>  template<typename T, unsigned N>
>>> 
>>> Modified: llvm/trunk/unittests/ADT/SmallVectorTest.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallVectorTest.cpp?rev=226899&r1=226898&r2=226899&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/unittests/ADT/SmallVectorTest.cpp (original)
>>> +++ llvm/trunk/unittests/ADT/SmallVectorTest.cpp Fri Jan 23 00:25:17 2015
>>> @@ -153,17 +153,14 @@ LLVM_ATTRIBUTE_USED void CompileTest() {
>>>    V.resize(42);
>>>  }
>>> 
>>> -// Test fixture class
>>> -template <typename VectorT>
>>> -class SmallVectorTest : public testing::Test {
>>> +class SmallVectorTestBase : public testing::Test {
>>>  protected:
>>> -  VectorT theVector;
>>> -  VectorT otherVector;
>>> 
>>>    void SetUp() {
>>>      Constructable::reset();
>>>    }
>>> 
>>> +  template <typename VectorT>
>>>    void assertEmpty(VectorT & v) {
>>>      // Size tests
>>>      EXPECT_EQ(0u, v.size());
>>> @@ -173,7 +170,8 @@ protected:
>>>      EXPECT_TRUE(v.begin() == v.end());
>>>    }
>>> 
>>> -  // Assert that theVector contains the specified values, in order.
>>> +  // Assert that v contains the specified values, in order.
>>> +  template <typename VectorT>
>>>    void assertValuesInOrder(VectorT & v, size_t size, ...) {
>>>      EXPECT_EQ(size, v.size());
>>> 
>>> @@ -188,6 +186,7 @@ protected:
>>>    }
>>> 
>>>    // Generate a sequence of values to initialize the vector.
>>> +  template <typename VectorT>
>>>    void makeSequence(VectorT & v, int start, int end) {
>>>      for (int i = start; i <= end; ++i) {
>>>        v.push_back(Constructable(i));
>>> @@ -195,6 +194,15 @@ protected:
>>>    }
>>>  };
>>> 
>>> +// Test fixture class
>>> +template <typename VectorT>
>>> +class SmallVectorTest : public SmallVectorTestBase {
>>> +protected:
>>> +  VectorT theVector;
>>> +  VectorT otherVector;
>>> +};
>>> +
>>> +
>>>  typedef ::testing::Types<SmallVector<Constructable, 0>,
>>>                           SmallVector<Constructable, 1>,
>>>                           SmallVector<Constructable, 2>,
>>> @@ -664,6 +672,67 @@ TYPED_TEST(SmallVectorTest, IteratorTest
>>>    this->theVector.insert(this->theVector.end(), L.begin(), L.end());
>>>  }
>>> 
>>> +template <typename InvalidType> class DualSmallVectorsTest;
>>> +
>>> +template <typename VectorT1, typename VectorT2>
>>> +class DualSmallVectorsTest<std::pair<VectorT1, VectorT2>> : public SmallVectorTestBase {
>>> +protected:
>>> +  VectorT1 theVector;
>>> +  VectorT2 otherVector;
>>> +
>>> +  template <typename T, unsigned N>
>>> +  static unsigned NumBuiltinElts(const SmallVector<T, N>&) { return N; }
>>> +};
>>> +
>>> +typedef ::testing::Types<
>>> +    // Small mode -> Small mode.
>>> +    std::pair<SmallVector<Constructable, 4>, SmallVector<Constructable, 4>>,
>>> +    // Small mode -> Big mode.
>>> +    std::pair<SmallVector<Constructable, 4>, SmallVector<Constructable, 2>>,
>>> +    // Big mode -> Small mode.
>>> +    std::pair<SmallVector<Constructable, 2>, SmallVector<Constructable, 4>>,
>>> +    // Big mode -> Big mode.
>>> +    std::pair<SmallVector<Constructable, 2>, SmallVector<Constructable, 2>>
>>> +  > DualSmallVectorTestTypes;
>>> +
>>> +TYPED_TEST_CASE(DualSmallVectorsTest, DualSmallVectorTestTypes);
>>> +
>>> +TYPED_TEST(DualSmallVectorsTest, MoveAssignment) {
>>> +  SCOPED_TRACE("MoveAssignTest-DualVectorTypes");
>>> +
>>> +  // Set up our vector with four elements.
>>> +  for (unsigned I = 0; I < 4; ++I)
>>> +    this->otherVector.push_back(Constructable(I));
>>> +
>>> +  const Constructable *OrigDataPtr = this->otherVector.data();
>>> +
>>> +  // Move-assign from the other vector.
>>> +  this->theVector =
>>> +    std::move(static_cast<SmallVectorImpl<Constructable>&>(this->otherVector));
>>> +
>>> +  // Make sure we have the right result.
>>> +  this->assertValuesInOrder(this->theVector, 4u, 0, 1, 2, 3);
>>> +
>>> +  // Make sure the # of constructor/destructor calls line up. There
>>> +  // are two live objects after clearing the other vector.
>>> +  this->otherVector.clear();
>>> +  EXPECT_EQ(Constructable::getNumConstructorCalls()-4,
>>> +            Constructable::getNumDestructorCalls());
>>> +
>>> +  // If the source vector (otherVector) was in small-mode, assert that we just
>>> +  // moved the data pointer over.
>>> +  EXPECT_TRUE(this->NumBuiltinElts(this->otherVector) == 4 ||
>>> +              this->theVector.data() == OrigDataPtr);
>>> +
>>> +  // There shouldn't be any live objects any more.
>>> +  this->theVector.clear();
>>> +  EXPECT_EQ(Constructable::getNumConstructorCalls(),
>>> +            Constructable::getNumDestructorCalls());
>>> +
>>> +  // We shouldn't have copied anything in this whole process.
>>> +  EXPECT_EQ(Constructable::getNumCopyConstructorCalls(), 0);
>>> +}
>>> +
>>>  struct notassignable {
>>>    int &x;
>>>    notassignable(int &x) : x(x) {}
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150123/785a9187/attachment.html>