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

[Ryosuke Niwa]

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.
>>
>>
> Don't recommend such checks. It is simply use-after-move. The expression
> WTFMove(x) means marking x as disposable.
>

That's simply not true. The semantics of WTFMove / std::move is really that
of a type cast. I think std::move was ill-named. It really should have been
called std::movable_cast or something. Also, as Geoff has already pointed
out, the behavior of STL doesn't prevent us from writing our own template
library to always have a very well specified & good state after
std::move'ed & its value was move-constructed.

It's one thing to consider that new WebKit contributors who are familiar
with C++ semantics of std::move and std::optional getting confused by
WebKit's implementation of std::option. But then our behavior of HashMap
which doesn't accept the POD integral value of 0 as a key, and how Ref and
RefPtr work and they're used by our WebCore code is probably more
problematic than this std::optional behavior.

In general, I find these kinds of dogmatic adoption of the best practices /
recommended ways of coding to be highly problematic. We shouldn't care who
or what entity recommends writing code in one way or another. We should be
questioning every and each recommendation anyone makes, and judging for
ourselves whether it makes a sense for the WebKit project.

For example, there is a group of people who swear by always wrapping each
single statement with { ~ } as a way of preventing bugs which stems from
forgetting { ~ } around multiple statements. Yet in my nine years of
writing & reviewing code in WebKit, I've never once came across such a
mistake made by either me or any other contributor.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Fujii Hironori]

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 expression
WTFMove(x) means marking x as disposable.

On Tue, Dec 18, 2018 at 8:55 AM Alex Christensen 
wrote:

>  I think we should do something to track and prevent the use of moved-from
> values instead of introducing our own std::optional replacement.
>

Do you have a idea?
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Maciej Stachowiak]

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, or an 
optional that doesn’t contain a value and says it doesn’t. 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.


> 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 useParameter(RefPtr&& param)
> {
>   RefPtr usedParam = WTFMove(param);
> }
> 
> void dontUseParameter(RefPtr&&) { }
> 
> int main() {
>   RefPtr a = adoptRef(new Test);
>   RefPtr 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 > > 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 > > 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 >> > 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 >>> > wrote:
 
 
 
> On Dec 17, 2018, at 11:10 AM, Chris Dumez  

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

[Geoffrey Garen]

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

(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  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 > > 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 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 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 > > 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 > > 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 useParameter(RefPtr&& param)
>> {
>>   RefPtr usedParam = WTFMove(param);
>> }
>> 
>> void dontUseParameter(RefPtr&&) { }
>> 
>> int main() {
>>   RefPtr a = adoptRef(new Test);
>>   RefPtr 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 

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

[Alex Christensen]

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  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 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 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  > 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  > 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 useParameter(RefPtr&& param)
> {
>   RefPtr usedParam = WTFMove(param);
> }
> 
> void dontUseParameter(RefPtr&&) { }
> 
> int main() {
>   RefPtr a = adoptRef(new Test);
>   RefPtr 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 > > 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 > > 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 

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

[Ryosuke Niwa]

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 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  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 
> 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 useParameter(RefPtr&& param)
>> {
>>   RefPtr usedParam = WTFMove(param);
>> }
>>
>> void dontUseParameter(RefPtr&&) { }
>>
>> int main() {
>>   RefPtr a = adoptRef(new Test);
>>   RefPtr 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  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  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 
>>> 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  wrote:
>>>
>>>
>>>
>>> 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 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

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

[Ryosuke Niwa]

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 
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 useParameter(RefPtr&& param)
> {
>   RefPtr usedParam = WTFMove(param);
> }
>
> void dontUseParameter(RefPtr&&) { }
>
> int main() {
>   RefPtr a = adoptRef(new Test);
>   RefPtr 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  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  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 
>> 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  wrote:
>>
>>
>>
>> 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 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 

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

[Chris Dumez]

> 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 useParameter(RefPtr&& param)
> {
>   RefPtr usedParam = WTFMove(param);
> }
> 
> void dontUseParameter(RefPtr&&) { }
> 
> int main() {
>   RefPtr a = adoptRef(new Test);
>   RefPtr 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.

Yes, this is a good example of case where std::exchange should be used or where 
the parameter should be a RefPtr and not a RefPtr&&. But as I 
mentioned earlier, the issue here is that the caller of WTFMove() is not 
actually calling any move constructor (it is merely doing a cast) and therefore 
cannot rely on the effects or a move constructor.
I do not think that this example is a good reason why we would not want 
optional to have a more predictable move constructor.


> 
>> On Dec 17, 2018, at 2:47 PM, Ryosuke Niwa > > 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 > > 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 >> > 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 >>> > wrote:
 
 
 
> On Dec 17, 2018, at 11:10 AM, Chris Dumez  > wrote:
> 
> 
> 

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

[Alex Christensen]

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 dontUseParameter(RefPtr&&) { }

int main() {
  RefPtr a = adoptRef(new Test);
  RefPtr 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  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  > 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 > > 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 >> > wrote:
>>> 
>>> 
>>> 
 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 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 

Re: [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  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 
> 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  wrote:
>
>
>
> 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 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

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

[Geoffrey Garen]

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

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

[Alex Christensen]

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

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

[Chris Dumez]

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

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

[Chris Dumez]

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


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Alex Christensen]

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

> On Dec 14, 2018, at 3:20 PM, youenn fablet  wrote:
> 
> Is it too late to ask for a std::optional change?
Yes

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Fujii Hironori]

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 we should add the move constructor to restore the valid null value
after move.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev