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