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