[PATCH] Move semantics for ValueMap

[Pete Cooper]

Hey David

How does this look for the move constructor/assignment operator?  I basically punted all of the actual work to just moving the members themselves, and trust that they have suitably defined move operators.

I’ll send out updated patches once we have something tenable, but didn’t want to throw this in to a later patch with tests until it looks good on its own.

Also, i’m happy to put a “Required because of MSVC’ type comment above these if you want it to be clear why they aren’t just ‘= default’.

Cheers,
Pete


  ValueMap(ValueMap &&other)
      : Map(std::move(other.Map)), MDMap(std::move(other.MDMap)),
        Data(std::move(other.Data)) {
  }
  ValueMap& operator=(ValueMap &&other) {
    Map = std::move(other.Map);
    MDMap = std::move(other.MDMap);
    Data = std::move(other.Data);
    return *this;
  }

> On Apr 27, 2015, at 10:41 AM, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> 
>> On Apr 27, 2015, at 10:37 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> Can we just drop all the special members from this type? (Rule of Zero)
> That would be even better.
>> 
>> Since it has a unique_ptr member it'll be move-only anyway.
>> 
>> (oh, except for MSVC, which doesn't synthesize move members - I don't
>> think it even works with explicit "= default" (check teh codebase to
>> see if there are any of those already - if there are, perhaps I'm
>> wrong?) so you might have to write them out explicitly)
> I did a check for ‘)= default’ and found some hits so figured MSVC was ok.  But now looking again I see those are only for C++03 constructors, destructors, and assignment operators.  I bet you’re right that MSVC will still struggle with this.
> 
> I’ll prepare another patch which should be MSVC safe.
>> 
>> Also: I'd drop the printfs from the test case - if they values are
>> interesting, just have the appropriate unit test check/EXPECT/whatever
>> for them. Then they're self-documenting without having to print them.
>> I can see, statically, in the code what the values are that they will
>> print.
> Thats fine.  You’re right about the values being self-documenting anyway.
> 
> Cheers,
> Pete
>> 
>> On Mon, Apr 27, 2015 at 9:59 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>>> Hi Rafael, David
>>> 
>>> In
>>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/273199.html,
>>> Rafael suggested returning a ValueMap from CloneFunction.
>>> 
>>> Turns out that ValueMap has a deleted copy constructor, and no move
>>> constructor all.  This patch adds the move constructor (and assignment
>>> operator). The default implementations of both of these seem to be fine.
>>> 
>>> To test this, i managed to get the Constructible class from the SmallVector
>>> unit test to wrap a Value*, then use that to count all the times the
>>> ValueMap does any of the operators.  I then went through the ValueMap and
>>> DenseMap code to verify each of the values i’m checking against.  I left
>>> some printf’s in an #if 0 for future use in case we want to easily check the
>>> values coming out of this code.
>>> 
>>> Cheers,
>>> Pete
>>> 
>>> 
>>> 
> 
> 
> _______________________________________________
> 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/20150428/c99d2237/attachment.html>