On 2016-04-28 9:52 AM, Gerald Squelart wrote:
> On Thursday, April 28, 2016 at 11:25:38 AM UTC+10, Ehsan Akhgari wrote:
>> On 2016-04-28 9:00 AM, Gerald Squelart wrote:
>>> On Thursday, April 28, 2016 at 10:41:21 AM UTC+10, Ehsan Akhgari wrote:
>>>> On 2016-04-28 8:00 AM, Gerald Squelart wrote:
>>>>> On Thursday, April 28, 2016 at 9:35:56 AM UTC+10, Kyle Huey wrote:
>>>>>> Can we catch this pattern with a compiler somehow?
>>>>>>
>>>>>> Foo foo;
>>>>>> foo.x = thing;
>>>>>> DoBar(mozilla::Move(foo));
>>>>>> if (foo.x) { /* do stuff */ }
>>>>
>>>> I think so. We already have an analysis which would detect whether the
>>>> return value of a function is used somewhere or not. We should be able
>>>> to reuse that to find the call to DoBar(), and then look for future
>>>> occurrences of foo used as an rvalue in the rest of the function. Once
>>>> we detect a use of "foo" as an lvalue, further usages of it as an rvalue
>>>> in the same function should be OK and not trigger the error. File a bug?
>>>>
>>>>> Definitely something that would be nice.
>>>>>
>>>>> But if we have/implement such a catcher, I'd like to have an annotation
>>>>> to say "yep I really want to reuse this moved-from object".
>>>>> Because sometimes the function will choose not to actually move from an
>>>>> rvalue-ref, or the object knows to revert to a fully-reusable state, etc.
>>>>
>>>> What you're describing sounds like a violation of move semantics, right?
>>>> The first case should only happen if DoBar doesn't accept an rvalue
>>>> reference, in which case the code above is definitely doing something
>>>> that the author did not expect, given that they have used Move(). The
>>>> latter case sounds completely broken, and if there is an actual good use
>>>> case for it, the C++ move semantics sound like the wrong tool to achieve
>>>> that goal to me.
>>>>
>>>> If you feel like I'm missing something or you can make a strong argument
>>>> on why breaking move semantics is OK in some cases, please let me know.
>>>> :-)
>>>>
>>>> Cheers,
>>>> Ehsan
>>>
>>> std::move and mozilla:Move are just casts that make an l-value object
>>> *look* like an r-value, so that when the compiler considers which 'DoBar'
>>> to use, one that takes an r-value reference will be picked first.
>>>
>>> "Move" is probably not the best name because it gives this impression that
>>> an actual move happens, but that's what we're stuck with in the standard.
>>
>> Yes, I understand that.
>>
>>> I don't see a "violation of move semantics" in there, could you please
>>> elaborate on what exact move semantics are violated?
>>> I'd say it's probably more a "perversion of the move spirit". :-)
>>
>> Even though Move() doesn't "actually" move anything, it has a meaning to
>> the author: "when I have an lvalue object, I want to move it to some
>> other place".
>>
>> While it's true that if DoBar() doesn't really move its argument
>> somewhere the code compiles, in that case DoBar() is breaking the
>> expectation of the caller since the caller clearly intended for a move
>> to happen. In C++ there is no way for the author to know that a move
>> actually happened, so the semantics of using the object after the move
>> are fuzzy. This is not really helpful since there is no clear way to
>> "check" whether a move actually happened, and it's not quite clear how
>> the caller should act if the callee decides to not move. IOW, without
>> something enforcing that a move actually happens, it is impossible to
>> understand the code Kyle posted above without knowing the implementation
>> of DoBar(), which is a problem with the C++ move semantics.
>>
>> However if we change the contract so that the callee is expected to
>> always perform a move, then we can perform a useful check on the caller
>> side to enforce not touching the object after it has been moved, which
>> is what Kyle was asking for.
>>
>>> In any case, a moved-from object *must* stay valid after that call, because
>>> at the minimum it will be destroyed at the end of its enclosing scope, by
>>> invoking its normal destructor, no magic or special path there.
>>
>> Define valid. The object will not be *destroyed* after a move, but it
>> should be "semantically empty" (the definition of "semantically empty"
>> is different depending on the class of the object.)
>>
>> I can see how it could be useful to for example call some methods on the
>> object such as IsEmpty(), but using the object willy-nilly is not OK
>> unless if we want to preserve the loose built-in C++ semantics where
>> Move() doesn't really mean much if anything unless if stars collide and
>> the callee and the caller both do something in concert. :-)
>>
>>> Now what to do with a moved-from object, is I think more a philosophical
>>> question! Some argue that we should do nothing (except the inevitable
>>> implied destruction). Others think it should be fine to also allow
>>> re-assignment (i.e. reuse the variable for something completely different).
>>> And yet others would allow total reuse.
>>
>> I don't think it's a philosophical question at all, quite to the
>> contrary it is a very practical question. Using such an object as an
>> lvalue is fine (the re-assignment case for example). Using the object
>> is any other way is not. Maybe you'll get away with that for *some*
>> classes, but we're talking about enforcing a more strict set of
>> semantics on our code which allows us to actually reason about what happens.
>
> Ok, maybe not philosophical, but definitely bikesheddingly a coding style
> issue which we (the Mozilla developers) should discuss and decide upon -- as
> I think is what is happening here. ;-)
>
>>> My position is that 'Move(x)' *usually* means we give the value away and
>>> don't want to use it again, and therefore a compiler warning/error would
>>> help catch unexpected reuses; but also that some situations call for reuse
>>> (e.g. the function doesn't always steal the object's contents, based on
>>> other factors) and a programmer in the know should be allowed to annotate
>>> this special case.
>>
>> Can you give a practical example of when a sane function would choose to
>> move some of the times but not others, and also explain how the caller
>> is supposed to know what to do, and also how it is supposed to know
>> whether the callee may not move in the first place?
>
> Say we have a resource held in a UniquePtr. Then I may have a number of
> potential suitors that could handle its content, so I will go through that
> list and for each, I will say 'take this if you want it', until someone
> actually takes it.
> E.g.:
> UniquePtr<MediaFile> file;
> for (auto& reader : mMediaReaders) {
> reader.TakeMediaIfKnown(file);
> if (!file) { // 'file' is now nullptr, reader eated it, we're done.
> break;
> }
> }
> if (file) {
> ReportFailure(file);
> }
> and a reader's method could look like:
> void MP4Reader::TakeMediaIfKnown(UniquePtr<MediaFile>&& aFile)
> {
> if (aFile && aFile->HasMimeType("video/mp4") {
> mMyFileUniquePtr = Move(aFile); // Actual move.
> ...
> }
> }
Hmm, OK, this is a good example. :-)
Even though Xidorn's suggestion may work in some cases, I can imagine
that in other cases you wouldn't want the caller to have to know the
preconditions of calling TakeMedia.
What about having a MaybeMove function that implementation wise is the
same as Move, but is exempt from the static check? That name has the
advantage of making it clear that a move _may_ happen, but not necessarily.
>>> Note that we talked a bit about this situation in:
>>> https://groups.google.com/d/topic/mozilla.dev.platform/VLtl2yL_TlA/discussion
>>> Referring to:
>>> http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#183
>>> Which talks about conditionally moving from a UniquePtr.
>>
>> I had missed that thread, but it seems like in that thread you're half
>> agreeing with me, unless I'm missing something? :-)
>
> I somewhat changed my mind. Can I change yours? ;-)
You kind of did. See above. :-)
> Anyway, how about this:
> - mozilla::Move() is reserved for expected moves, and *any* re-use after that
> is considered an error.
> - We introduce something like mozilla::MakeAvailableForMove(), which allows
> for re-use.
> - For extra safety, we could allow MakeAvailableForMove() to only work on
> classes that have a special attribute, e.g. MOZ_TYPE_IS_REUSABLE_AFTER_MOVE.
What's the semantics of MakeAvailableForMove()? Something similar to my
MaybeMove suggestion?
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
