On Tue, Dec 18, 2018 at 3:18 AM Geoffrey Garen <gga...@apple.com> wrote:
> 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”; > For example, std::unique_ptr does have a fully specified moved-from state that can be relied on. antti > (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> 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> 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> >> 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> 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> 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> >>>> 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 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> wrote: >>>> >>>> >>>> >>>> On Dec 17, 2018, at 11:10 AM, Chris Dumez <cdu...@apple.com> wrote: >>>> >>>> >>>> >>>> On Dec 17, 2018, at 10:27 AM, Alex Christensen <achristen...@apple.com> >>>> wrote: >>>> >>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez <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 >>>> >>>> >>>> _______________________________________________ >>>> 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 >>>> >>> >>> > _______________________________________________ > 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 >
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev