[PATCH] Move semantics for ValueMap

[David Blaikie]

On Tue, Apr 28, 2015 at 11:42 AM, Pete Cooper <peter_cooper at apple.com>
wrote:

> 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.
>

Yep, that's the right idea.


> 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’.
>

I'm not too fussed about commenting it. We have so many of these already &
will only have more. Eventually we'll switch to some version of MSVC that
supports the defaults & we'll just remove all of them.


>
> 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/43af3c96/attachment.html>