Right now, after a maybeMove type call on std::optional, you get either the
original value, or a value that is officially valid but unspecified, and
actually an optional that says it contains a value but doesn’t. With Chris’s
proposal, you’d get a WTF::Optional with either the original value, or an
optional that doesn’t contain a value and says it doesn’t. Among other things,
this allows for a “did anything actually get left here” check after the
function that may or may not move a value. Seems like an upgrade.
> On Dec 17, 2018, at 3:55 PM, Alex Christensen <achristen...@apple.com> wrote:
>
> Let me give a concrete example on why, even with our nice-to-use WTF types,
> the state of a C++ object is undefined after being moved from:
>
> #include <wtf/RefCounted.h>
> #include <wtf/RefPtr.h>
> #include <iostream>
>
> class Test : public RefCounted<Test> { };
>
> void useParameter(RefPtr<Test>&& param)
> {
> RefPtr<Test> usedParam = WTFMove(param);
> }
>
> void dontUseParameter(RefPtr<Test>&&) { }
>
> int main() {
> RefPtr<Test> a = adoptRef(new Test);
> RefPtr<Test> b = adoptRef(new Test);
> std::cout << "a null? " << !a << std::endl;
> std::cout << "b null? " << !b << std::endl;
> useParameter(WTFMove(a));
> dontUseParameter(WTFMove(b));
> std::cout << "a null? " << !a << std::endl;
> std::cout << "b null? " << !b << std::endl;
> return 0;
> }
>
> // clang++ test.cpp -I Source/WTF -L WebKitBuild/Debug -l WTF -framework
> Foundation -L /usr/lib -l icucore --std=c++17 && ./a.out
>
> // a null? 0
>
> // b null? 0
>
> // a null? 1
>
> // b null? 0
>
>
>
> As you can see, the internals of callee dontUseParameter (which could be in a
> different translation unit) affects the state of the local variable b in this
> function. This is one of the reasons why the state of a moved-from variable
> is intentionally undefined, and we can’t fix that by using our own
> std::optional replacement. If we care about the state of a moved-from
> object, that is what std::exchange is for. I think we should do something to
> track and prevent the use of moved-from values instead of introducing our own
> std::optional replacement.
>
>> On Dec 17, 2018, at 2:47 PM, Ryosuke Niwa <rn...@webkit.org
>> <mailto:rn...@webkit.org>> wrote:
>>
>> Yeah, it seems like making std::optional more in line with our own
>> convention provides more merits than downsides here. People are using
>> WTFMove as if it's some sort of a swap operation in our codebase, and as
>> Maciej pointed out, having rules where people have to think carefully as to
>> when & when not to use WTFMove seems more troublesome than the proposed fix,
>> which would mean this work for optional.
>>
>> - R. Niwa
>>
>> On Mon, Dec 17, 2018 at 2:24 PM Geoffrey Garen <gga...@apple.com
>> <mailto:gga...@apple.com>> wrote:
>> I don’t understand the claim about “undefined behavior” here. As Maciej
>> pointed out, these are our libraries. We are free to define their behaviors.
>>
>> In general, “undefined behavior” is an unwanted feature of programming
>> languages and libraries, which we accept begrudgingly simply because there
>> are practical limits to what we can define. This acceptance is not a mandate
>> to carry forward undefined-ness as a badge of honor. In any case where it
>> would be practical to define a behavior, that defined behavior would be
>> preferable to undefined behavior.
>>
>> I agree that the behavior of move constructors in the standard library is
>> undefined. The proposal here, as I understand it, is to (a) define the
>> behaviors move constructors in WebKit and (b) avoid std::optional and use an
>> optional class with well-defined behavior instead.
>>
>> Because I do not ❤️ security updates, I do ❤️ defined behavior, and so I ❤️
>> this proposal.
>>
>> Geoff
>>
>>> On Dec 17, 2018, at 12:50 PM, Alex Christensen <achristen...@apple.com
>>> <mailto:achristen...@apple.com>> wrote:
>>>
>>> This one and the many others like it are fragile, relying on undefined
>>> behavior, and should be replaced by std::exchange. Such a change was made
>>> in https://trac.webkit.org/changeset/198755/webkit
>>> <https://trac.webkit.org/changeset/198755/webkit> and we probably need many
>>> more like that, but we are getting away with relying on undefined behavior
>>> which works for us in most places.
>>>
>>>> On Dec 17, 2018, at 11:24 AM, Chris Dumez <cdu...@apple.com
>>>> <mailto:cdu...@apple.com>> wrote:
>>>>
>>>>
>>>>
>>>>> On Dec 17, 2018, at 11:10 AM, Chris Dumez <cdu...@apple.com
>>>>> <mailto:cdu...@apple.com>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On Dec 17, 2018, at 10:27 AM, Alex Christensen <achristen...@apple.com
>>>>>> <mailto:achristen...@apple.com>> wrote:
>>>>>>
>>>>>>>>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdu...@apple.com
>>>>>>>>>> <mailto:cdu...@apple.com>> wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> As far as I know, our convention in WebKit so far for our types has
>>>>>>>>>> been that types getting moved-out are left in a valid “empty” state.
>>>>>> This is not necessarily true. When we move out of an object to pass
>>>>>> into a function parameter, for example, the state of the moved-from
>>>>>> object depends on the behavior of the callee. If the callee function
>>>>>> uses the object, we often have behavior that leaves the object in an
>>>>>> “empty” state of some kind, but we are definitely relying on fragile
>>>>>> undefined behavior when we do so because changing the callee to not use
>>>>>> the parameter changes the state of the caller. We should never assume
>>>>>> that WTFMove or std::move leaves the object in an empty state. That is
>>>>>> always a bug that needs to be replaced by std::exchange.
>>>>>
>>>>> Feel like we’re taking about different things. I am talking about move
>>>>> constructors (and assignment operators), which have a well defined
>>>>> behavior in WebKit. And it seems you are talking about WTFMove(), which
>>>>> despite the name does not “move” anything, it is merely a cast.
>>>>> In the case you’re talking about the caller does NOT call the move
>>>>> constructor, it merely does a cast so I do not think your comment
>>>>> invalidates my statement. Note that in my patch, I was nearly
>>>>> WTFMove()ing the data member and assigning it to a local variable right
>>>>> away, calling the move constructor.
>>>>
>>>> Also note that may of us already rely on our move constructors’ behavior,
>>>> just search for WTFMove(m_responseCompletionHandler) in:
>>>> https://trac.webkit.org/changeset/236463/webkit
>>>> <https://trac.webkit.org/changeset/236463/webkit>
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org>
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev>
>>
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org>
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> <https://lists.webkit.org/mailman/listinfo/webkit-dev>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
