[webkit-dev] Watch out for std::optional's move constructor

[Ryosuke Niwa]

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 <ggaren at 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 <achristensen at 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 <cdumez at apple.com> wrote:
>
>
>
> On Dec 17, 2018, at 11:10 AM, Chris Dumez <cdumez at apple.com> wrote:
>
>
>
> On Dec 17, 2018, at 10:27 AM, Alex Christensen <achristensen at apple.com>
> wrote:
>
> On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at 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 at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20181217/4ca7c80a/attachment.html>