Again, the C++ standard does not say that moving from an object leaves the object in an undefined state.
The C++ standard does say: > Objects of types defined in the C++ standard library may be moved from > (12.8). Move operations may be explicitly specified or implicitly generated. > Unless otherwise specified, such moved-from objects shall be placed in a > valid but unspecified state. A few ways this differs from the claim that moving from an object leaves the object in an undefined state: (1) The standard makes no mention of objects in general, only of objects in the C++ standard library; (2) Even for objects in the C++ standard library, the standard makes no mention of objects in general, only of objects that are not “otherwise specified”; (3) The standard avoids undefined-ness entirely and instead specifies a valid but unspecified state. I think it is accurate to say that the C++ standard specifies that some objects in the standard library have valid but unspecified state when moved from. I think it’s also accurate to say that a function that accepts an rvalue reference as an argument does not promise to move from that rvalue reference. I think both of these statements are compatible with specifying what happens in WebKit libraries when we do move from an object. Geoff > On Dec 17, 2018, at 5:04 PM, Alex Christensen <achristen...@apple.com> wrote: > > We can definitely make our own WTF::Optional instead of using std::optional > and change its move constructor/operator= and I personally think that would > not be worth it but if I’m in the minority I’ll deal with it. We cannot > diverge from the C++ standards that say that moving from an object leaves the > object in an undefined state, and right now in WebKit we are relying quite a > lot on that undefined state. I think that is the bigger problem that Chris > was trying to solve. > >> On Dec 17, 2018, at 4:32 PM, Ryosuke Niwa <rn...@webkit.org >> <mailto:rn...@webkit.org>> wrote: >> >> Let me add this. >> >> The situation we have here is analogous to having a member function "move()" >> which leave std::optional in undefined state as in: >> >> std::optional<int> a; >> std::optional<int> b = a.move(); >> >> would leave a in some undefined state. I don't care what C++ standards say >> or what STL does. That's insane. >> Every object should remain in a well defined state after performing an >> operation. >> >> - R. Niwa >> >> >> On Mon, Dec 17, 2018 at 4:22 PM Ryosuke Niwa <rn...@webkit.org >> <mailto:rn...@webkit.org>> wrote: >> It's true that WTFMove or std::move doesn't do anything if the moved >> variable is not used because WTFMove / std::move is just a type cast. >> >> However, that behavior is orthogonal from the issue that calling WTFMove / >> std::move on std::optional, and the returned value is assigned to another >> std::optional, the original std::optional will be left a bad state. >> >> I completely disagree with your assessment that this calls for the use of >> std::exchange. >> >> >> On Mon, Dec 17, 2018 at 3:55 PM Alex Christensen <achristen...@apple.com >> <mailto: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
