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

On Mon, Dec 17, 2018 at 6:40 PM Fujii Hironori wrote: > > On Tue, Dec 18, 2018 at 11:16 AM Maciej Stachowiak wrote: > >> >> Among other things, this allows for a “did anything actually get left >> here” check after the function that may or may not move a value. Seems like >> an upgrade. >> >>

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

On Tue, Dec 18, 2018 at 11:16 AM Maciej Stachowiak wrote: > > Among other things, this allows for a “did anything actually get left > here” check after the function that may or may not move a value. Seems like > an upgrade. > > Don't recommend such checks. It is simply use-after-move. The

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

Right now, after a maybeMove type call on std::optional, you get either the original value, or a value that is officially valid but unspecified, and actually an optional that says it contains a value but doesn’t. With Chris’s proposal, you’d get a WTF::Optional with either the original value,

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

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

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

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

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

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 a; std::optional b = a.move(); would leave a in some undefined state. I don't care what C++ standards say or what STL does. That's

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

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

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

> On Dec 17, 2018, at 3:55 PM, Alex Christensen 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 > #include > #include > > class Test : public RefCounted { }; > > void

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

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 #include #include class Test : public RefCounted { }; void useParameter(RefPtr&& param) { RefPtr usedParam = WTFMove(param); } void

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

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

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

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

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

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

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

> On Dec 17, 2018, at 11:10 AM, Chris Dumez wrote: > > > >> On Dec 17, 2018, at 10:27 AM, Alex Christensen > > wrote: >> >> On Dec 14, 2018, at 1:37 PM, Chris Dumez > > wrote: > >> >> As far as I know, our

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

> On Dec 17, 2018, at 10:27 AM, Alex Christensen wrote: > > On Dec 14, 2018, at 1:37 PM, Chris Dumez > 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

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

On Dec 14, 2018, at 1:37 PM, Chris Dumez >>> > 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

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

On Mon, Dec 17, 2018 at 4:55 PM Maciej Stachowiak wrote: > > Maybe there is an option I am missing? Out of these, I think (2) is likely > the best, on the whole. > > Reasonable. But, some classes need to be modification for that. For example, WTF::URL is using default move constructor. I think