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

[Fujii Hironori]

On Wed, Dec 19, 2018 at 6:41 AM Konstantin Tokarev 
wrote:

>
> I agree that "atomic" part of AtomicString is kinda misleading, however
> wiki
> explains it all
>
> https://trac.webkit.org/wiki/EfficientStrings#AtomicStringVSString
>
> BTW, /me personally didn't know what "interned string" is until today :)
>

The return value of RegisterClass Win32 API is ATOM.
And, Lisp has the similar concepts of intern and atom.
I've never thought AtomicString is a confusing name until I see you
comments today.
It is a intrinsic name for me as a Win32 and Lisp programmer.

https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-addatoma
https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-registerclassa
___
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

[Konstantin Tokarev]

18.12.2018, 22:35, "Michael Catanzaro" :
> I know I'm getting a bit far afield here, but:
>
> On Mon, Dec 17, 2018 at 9:26 PM, Ryosuke Niwa  wrote:
>>  But then our behavior of HashMap which doesn't accept the POD
>>  integral value of 0 as a key
>
> This behavior is really unexpected and dangerous [1], and we should
> seriously consider changing it. No doubt lots of bugs caused by this
> are just waiting to be uncovered. I've been working on WebKit since
> 2014 and didn't know about this until last month.
>
> Another oddity: I recently learned that AtomicStrings are actually
> interned strings. WTF. Why not call them that? I had thought for years
> that they were strings safe to be shared across threads, like other
> atomics. Not at all. Maybe this was dumb of me, but it could have been
> avoided by better naming.

I agree that "atomic" part of AtomicString is kinda misleading, however wiki
explains it all

https://trac.webkit.org/wiki/EfficientStrings#AtomicStringVSString

BTW, /me personally didn't know what "interned string" is until today :)

>
> Michael
>
> [1] https://trac.webkit.org/changeset/238407/webkit
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-- 
Regards,
Konstantin

___
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

[Ryosuke Niwa]

On Tue, Dec 18, 2018 at 11:35 AM Michael Catanzaro 
wrote:

> I know I'm getting a bit far afield here, but:
>
> On Mon, Dec 17, 2018 at 9:26 PM, Ryosuke Niwa  wrote:
> > But then our behavior of HashMap which doesn't accept the POD
> > integral value of 0 as a key
>
> This behavior is really unexpected and dangerous [1], and we should
> seriously consider changing it. No doubt lots of bugs caused by this
> are just waiting to be uncovered. I've been working on WebKit since
> 2014 and didn't know about this until last month.


I tend to agree but then we'd come up with other numbers for the empty &
deleted values.
I've been thinking that we could use -1 and -2 but that's also somewhat
arbitrary restriction.

Another oddity: I recently learned that AtomicStrings are actually
> interned strings. WTF. Why not call them that? I had thought for years
> that they were strings safe to be shared across threads, like other
> atomics. Not at all. Maybe this was dumb of me, but it could have been
> avoided by better naming.
>

This topic has been discussed extensively in the past:
https://lists.webkit.org/pipermail/webkit-dev/2013-June/024988.html

- 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

[Michael Catanzaro]

I know I'm getting a bit far afield here, but:

On Mon, Dec 17, 2018 at 9:26 PM, Ryosuke Niwa  wrote:
But then our behavior of HashMap which doesn't accept the POD 
integral value of 0 as a key


This behavior is really unexpected and dangerous [1], and we should 
seriously consider changing it. No doubt lots of bugs caused by this 
are just waiting to be uncovered. I've been working on WebKit since 
2014 and didn't know about this until last month.


Another oddity: I recently learned that AtomicStrings are actually 
interned strings. WTF. Why not call them that? I had thought for years 
that they were strings safe to be shared across threads, like other 
atomics. Not at all. Maybe this was dumb of me, but it could have been 
avoided by better naming.


Michael

[1] https://trac.webkit.org/changeset/238407/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

[Fujii Hironori]

On Tue, Dec 18, 2018 at 10:21 PM Fujii Hironori 
wrote:

>
> On Tue, Dec 18, 2018 at 12:25 PM Ryosuke Niwa 
> wrote:
>
>> 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.
>>
>
> Do you mean only WTF classes should guarantee the valid empty state after
> moved-out?
> Should we use std::exchange and std::move properly for other classes in
> WebKit?
>
>

I guess the answer is all classes in WebKit which are moved by WTFMove.
I'm wondering how much runtime cost it will take to restore valid empty
state of all moved-out values.
___
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 12:25 PM Ryosuke Niwa 
wrote:

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

Do you mean only WTF classes should guarantee the valid empty state after
moved-out?
Should we use std::exchange and std::move properly for other classes in
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

[Antti Koivisto]

On Tue, Dec 18, 2018 at 3:18 AM Geoffrey Garen  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 
> 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 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 

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

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

[Maciej Stachowiak]

> On Dec 16, 2018, at 8:37 PM, Chris Dumez  wrote:
> 
> 
>> On Dec 16, 2018, at 7:43 PM, Fujii Hironori  wrote:
>> 
>> I don't like the proposal because it encourages misuse of move.
>> We can use move only for values about to be destroyed.
> 
> Just for reference, there are close to 400 matches for "WTFMove(m_” in our 
> code base. People do seem to rely on the state of objects after being moved 
> out.
> I totally agree that the state of the object being moved out is not defined 
> by the C++ standard. However, so far, in WebKit, we’ve been careful with our 
> move-constructors.
> 
> I think that if we do not update std::optional’s move constructor, then I 
> worry we’ll keep having to fix bugs in the future due to its misuse. 
> Although, maybe this mail thread will help.
> 
> That being said, I agree with your and Daniel and we should use std::exchange 
> more. I think all the "WTFMove(m_” lines in our code bases should probably be 
> replaced with std::exchange.

I think it would be easier to enforce a rule of “always use WTF::Optional 
instead of std::optional” than a rule of “use std::exchange more, but sometimes 
you really need WTFMove(), but don’t use move when it would be wrong to do so”. 
Better to set up the code to create simple rules that don’t require judgment 
IMO.

> 
> 
>> 
>> I like Dan's suggestion. We should use std::exchange or std::optional::swap 
>> for the cases.
>> Or, what about adding a new method WTF::Optional::release() for the case?
>> 
>> ___
>> 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

[Maciej Stachowiak]

> On Dec 16, 2018, at 3:47 PM, Daniel Bates  wrote:
> 
> 
> 
>> On Dec 14, 2018, at 3:52 PM, Chris Dumez > > wrote:
>> 
>> So to be clear, it is often not truly about using the value after it is 
>> moved. It is about expecting that the variable / member has been nulled out 
>> after moving it.
>> If I WTFMove() out a data member m_dataMember, I expect later on `if 
>> (m_dataMember)` to be false.
>> 
> 
> I do not mean to derail the discussion about whether or not we should have 
> our own std::optional that disengages on move. I am all for convenience. I 
> think expecting a valid “empty” state (*) when moving out a data member is an 
> anti-pattern and I propose that we should be using std::exchange() instead 
> (or any code analogous to doing std::exchange()) for these cases, including 
> the case in your original email and especially with data members as in 
>  >.

Expectations can’t be an anti-pattern, only code can be an anti-pattern. At the 
point of checking whether a value is “empty” by some definition, you may not 
know that someone has std::move()d it, and shouldn’t have to. So to deal with 
the bug-prone nature of check after std::move(), we can do one of these:

(1) Ban all use of std::move() unless it’s possible for some sort of static 
check to know that no one will examine the moved value
(2) Make sure we only use types that leave themselves in a state that meets 
expectations post-move().
(3) Try real hard not to make mistakes and hope for the best

Maybe there is an option I am missing? Out of these, I think (2) is likely the 
best, on the whole.

> 
> Why do I consider it an anti-pattern? See (*) and because the concept of 
> “moving" is not spec'ed to behave like this. Here’s one reference to the 
> spec’ed behavior and an example comparable to the one in your first email 
> (highlighting is my own for emphasis):
> 
> [[
> Unless otherwise specified, all standard library objects that have been moved 
> from are placed in a valid but unspecified state. That is, only the functions 
> without preconditions, such as the assignment operator, can be safely used on 
> the object after it was moved from:

Note that this is specified for “standard library objects”. There’s nothing 
wrong with choosing to give a stronger API contract for our own objects, and 
then to rely on it.

> std::vector  > v;
> std::string  str = 
> "example";
> v.push_back(std::move(str)); // str is now valid but unspecified
> str.back(); // undefined behavior if size() == 0: back() has a precondition 
> !empty()
> str.clear(); // OK, clear() has no preconditions
> 
> ]]
>  >
> 
> (*) What is the valid “empty” state for a type without a default constructor?
> 
> Dan
> 
>> --
>>  Chris Dumez
>> 
>> 
>> 
>> 
>>> On Dec 14, 2018, at 3:45 PM, Chris Dumez >> > wrote:
>>> 
>>> 
 On Dec 14, 2018, at 3:39 PM, Fujii Hironori >>> > wrote:
 
 
 On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez >>> > wrote:
 
 I have now been caught twice by std::optional’s move constructor. 
 
  I don't understand how this could be useful? Do you want to use the value 
 after it is moved? I'd like to see these your code. Could you show me 
 these two patches?
>>> 
>>> This is the latest one: https://trac.webkit.org/changeset/239228/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

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

[Fujii Hironori]

I've changed my mind.

It is ensured std::vector move constructor makes moved value empty.

> After the move, other is guaranteed to be empty().

https://en.cppreference.com/w/cpp/container/vector/vector

Unfortunately, WTF::Vector move constructor just swaps values.
I think this swapping is not a specified behavior WebKit developers can
rely on.

https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Vector.h?rev=238031#L950

I think WTF::Vector also should ensure moved values become empty.

Looking through the grepping "WTFMove(m_”result, some of them are valid
because:
1. It is in dtor. 'this' object is about to be destructed.
2. It is in ref-qualified method. 'this' object is about to be destructed.
3. It is Ref<>. As well as std::shared_ptr, it should be defined to become
null after moved.

Common misuse cases are moving Vector like following code.

void SVGDocumentExtensions::rebuildElements()
> {
> Vector shadowRebuildElements = WTFMove(m_rebuildElements);


I thought std::exchange should be used for these purpose.
But, if WTF::Vector move constructor is changed, this code would be valid.
m_rebuildElements would become empty as expected, not accidentally.

If above code would be valid, there would be no reason to object we have
custom Optional class which guarantees moved values become empty.


BWT, FWIW,
clang-tidy has use-after-move checker.

clang-tidy - bugprone-use-after-move — Extra Clang Tools 8 documentation
http://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html


As expected, it doesn't support member variables.

The check currently only considers calls of std::move on local variables or
> function parameters. It does not check moves of member variables or global
> variables.


It seems UBSan doesn't check use-after-move.

UndefinedBehaviorSanitizer — Clang 8 documentation
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks
___
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 16, 2018, at 7:43 PM, Fujii Hironori  wrote:
> 
> I don't like the proposal because it encourages misuse of move.
> We can use move only for values about to be destroyed.

Just for reference, there are close to 400 matches for "WTFMove(m_” in our code 
base. People do seem to rely on the state of objects after being moved out.
I totally agree that the state of the object being moved out is not defined by 
the C++ standard. However, so far, in WebKit, we’ve been careful with our 
move-constructors.

I think that if we do not update std::optional’s move constructor, then I worry 
we’ll keep having to fix bugs in the future due to its misuse. Although, maybe 
this mail thread will help.

That being said, I agree with your and Daniel and we should use std::exchange 
more. I think all the "WTFMove(m_” lines in our code bases should probably be 
replaced with std::exchange.


> 
> I like Dan's suggestion. We should use std::exchange or std::optional::swap 
> for the cases.
> Or, what about adding a new method WTF::Optional::release() for the case?
> 
> ___
> 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

[Fujii Hironori]

I don't like the proposal because it encourages misuse of move.
We can use move only for values about to be destroyed.

I like Dan's suggestion. We should use std::exchange or std::optional::swap
for the cases.
Or, what about adding a new method WTF::Optional::release() for the case?
___
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

[Daniel Bates]

> On Dec 14, 2018, at 3:52 PM, Chris Dumez  > wrote:
> 
> So to be clear, it is often not truly about using the value after it is 
> moved. It is about expecting that the variable / member has been nulled out 
> after moving it.
> If I WTFMove() out a data member m_dataMember, I expect later on `if 
> (m_dataMember)` to be false.
> 

I do not mean to derail the discussion about whether or not we should have our 
own std::optional that disengages on move. I am all for convenience. I think 
expecting a valid “empty” state (*) when moving out a data member is an 
anti-pattern and I propose that we should be using std::exchange() instead (or 
any code analogous to doing std::exchange()) for these cases, including the 
case in your original email and especially with data members as in 
>.

Why do I consider it an anti-pattern? See (*) and because the concept of 
“moving" is not spec'ed to behave like this. Here’s one reference to the 
spec’ed behavior and an example comparable to the one in your first email 
(highlighting is my own for emphasis):

[[
Unless otherwise specified, all standard library objects that have been moved 
from are placed in a valid but unspecified state. That is, only the functions 
without preconditions, such as the assignment operator, can be safely used on 
the object after it was moved from:
std::vector http://en.cppreference.com/w/cpp/string/basic_string>> v;
std::string  str = 
"example";
v.push_back(std::move(str)); // str is now valid but unspecified
str.back(); // undefined behavior if size() == 0: back() has a precondition 
!empty()
str.clear(); // OK, clear() has no preconditions

]]
>

(*) What is the valid “empty” state for a type without a default constructor?

Dan

> --
>  Chris Dumez
> 
> 
> 
> 
>> On Dec 14, 2018, at 3:45 PM, Chris Dumez > > wrote:
>> 
>> 
>>> On Dec 14, 2018, at 3:39 PM, Fujii Hironori >> > wrote:
>>> 
>>> 
>>> On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez >> > wrote:
>>> 
>>> I have now been caught twice by std::optional’s move constructor. 
>>> 
>>>  I don't understand how this could be useful? Do you want to use the value 
>>> after it is moved? I'd like to see these your code. Could you show me these 
>>> two patches?
>> 
>> This is the latest one: https://trac.webkit.org/changeset/239228/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]

> This expression WTFMove(*m_pendingWebsitePolicies) doesn't move 
> std::optional, but moves the content of the 
> std::optional, WebsitePoliciesData. I think your proposal doesn't work for 
> this code.

The original code, which asserted, did this:

if (auto pendingWebsitePolicies = WTFMove(m_pendingWebsitePolicies))

WebsitePoliciesData::applyToDocumentLoader(WTFMove(*pendingWebsitePolicies), 
documentLoader);

The relevant expression was "WTFMove(m_pendingWebsitePolicies)”, which did move 
std::optional. The expectation in the code was that, after 
WTFMove(m_pendingWebsitePolicies), m_pendingWebsitePolicies would be empty.

Geoff___
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

[David Kilzer]

Filed:  

Dave


On Dec 14, 2018, at 2:18 PM, Adrian Perez de Castro  wrote:

> Hello,
> 
> On Fri, 14 Dec 2018 14:02:39 -0800, Saam Barati  wrote:
> 
>>> On Dec 14, 2018, at 1:59 PM, Chris Dumez  wrote:
>>> 
 On Dec 14, 2018, at 1:56 PM, Saam Barati >>> > wrote:
 
> On Dec 14, 2018, at 1:54 PM, Saam Barati  > wrote:
> 
>> On Dec 14, 2018, at 1:37 PM, Chris Dumez > > wrote:
>> 
>> Hi,
>> 
>> I have now been caught twice by std::optional’s move constructor. It 
>> turns out that it leaves the std::optional being moved-out *engaged*, it 
>> merely moves its value.
>> 
>> For example, testOptional.cpp:
>> #include 
>> #include 
>> 
>> int main()
>> {
>>   std::optional a = 1;
>>   std::optional b = std::move(a);
>>   std::cout << "a is engaged? " << !!a << std::endl;
>>   std::cout << "b is engaged? " << !!b << std::endl;
>>   return 0;
>> }
>> 
>> $ clang++ testOptional.cpp -o testOptional -std=c++17
>> $ ./testOptional
>> a is engaged? 1
>> b is engaged? 1
>> 
>> I would have expected:
>> a is engaged? 0
>> b is engaged? 1
> I would have expected this too.
> 
> This is also what I would have expected.
> 
>> This impacts the standard std::optional implementation on my machine as 
>> well as the local copy in WebKit’s wtf/Optional.h.
>> 
>> 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.
> I believe it's UB to use an object after it has been moved.
 I'm wrong here.
 Apparently objects are left in a "valid but unspecified state".
 
 https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove
  
 
>>> I believe in WebKit, however, we’ve made sure our types are left in a valid 
>>> “empty” state, thus my proposal for our own optional type that would be 
>>> less error-prone / more convenient to use.
>> I think your proposal is reasonable. I agree it's less error prone.
>> 
>> I think if we make this change, we should pull optional out of std and put 
>> it in WTF, since we're now implementing behavior we will rely on being 
>> specified.
> 
> I am also in favor of having an implementation in WTF that empties the
> optional after moving the value out from it.
> 
> Cheers,
> 
> 
> -Adrián
> ___
> 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

[Fujii Hironori]

On Sat, Dec 15, 2018 at 8:46 AM Chris Dumez  wrote:

>
>
> This is the latest one: https://trac.webkit.org/changeset/239228/webkit
>

This expression WTFMove(*m_pendingWebsitePolicies) doesn't move
std::optional, but moves the content of the
std::optional, WebsitePoliciesData. I think your proposal doesn't work for
this code.
___
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]

So to be clear, it is often not truly about using the value after it is moved. 
It is about expecting that the variable / member has been nulled out after 
moving it.
If I WTFMove() out a data member m_dataMember, I expect later on `if 
(m_dataMember)` to be false.

--
 Chris Dumez




> On Dec 14, 2018, at 3:45 PM, Chris Dumez  wrote:
> 
> 
>> On Dec 14, 2018, at 3:39 PM, Fujii Hironori > > wrote:
>> 
>> 
>> On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez > > wrote:
>> 
>> I have now been caught twice by std::optional’s move constructor. 
>> 
>>  I don't understand how this could be useful? Do you want to use the value 
>> after it is moved? I'd like to see these your code. Could you show me these 
>> two patches?
> 
> This is the latest one: https://trac.webkit.org/changeset/239228/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

[Chris Dumez]

> On Dec 14, 2018, at 3:39 PM, Fujii Hironori  wrote:
> 
> 
> On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez  > wrote:
> 
> I have now been caught twice by std::optional’s move constructor. 
> 
>  I don't understand how this could be useful? Do you want to use the value 
> after it is moved? I'd like to see these your code. Could you show me these 
> two patches?

This is the latest one: https://trac.webkit.org/changeset/239228/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

[youenn fablet]

Is it too late to ask for a std::optional change?

Le ven. 14 déc. 2018 à 13:37, Chris Dumez  a écrit :
>
> Hi,
>
> I have now been caught twice by std::optional’s move constructor. It turns 
> out that it leaves the std::optional being moved-out *engaged*, it merely 
> moves its value.
>
> For example, testOptional.cpp:
> #include 
> #include 
>
> int main()
> {
> std::optional a = 1;
> std::optional b = std::move(a);
> std::cout << "a is engaged? " << !!a << std::endl;
> std::cout << "b is engaged? " << !!b << std::endl;
> return 0;
> }
>
> $ clang++ testOptional.cpp -o testOptional -std=c++17
> $ ./testOptional
> a is engaged? 1
> b is engaged? 1
>
> I would have expected:
> a is engaged? 0
> b is engaged? 1
>
> This impacts the standard std::optional implementation on my machine as well 
> as the local copy in WebKit’s wtf/Optional.h.
>
> 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.
> As such, I find that std::optional’s move constructor behavior is error-prone.
>
> I’d like to know how do other feel about this behavior? If enough people 
> agree this is error-prone, would we consider having our
> own optional type in WTF which resets the engaged flag (and never allow the 
> std::optional)?
>
> Thanks,
> --
>  Chris Dumez
>
>
>
>
> ___
> 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

[Adrian Perez de Castro]

Hello,

On Fri, 14 Dec 2018 14:02:39 -0800, Saam Barati  wrote:
 
> > On Dec 14, 2018, at 1:59 PM, Chris Dumez  wrote:
> > 
> >> On Dec 14, 2018, at 1:56 PM, Saam Barati  >> > wrote:
> >> 
> >>> On Dec 14, 2018, at 1:54 PM, Saam Barati  >>> > wrote:
> >>> 
>  On Dec 14, 2018, at 1:37 PM, Chris Dumez   > wrote:
>  
>  Hi,
>  
>  I have now been caught twice by std::optional’s move constructor. It 
>  turns out that it leaves the std::optional being moved-out *engaged*, it 
>  merely moves its value.
>  
>  For example, testOptional.cpp:
>  #include 
>  #include 
>  
>  int main()
>  {
>  std::optional a = 1;
>  std::optional b = std::move(a);
>  std::cout << "a is engaged? " << !!a << std::endl;
>  std::cout << "b is engaged? " << !!b << std::endl;
>  return 0;
>  }
>  
>  $ clang++ testOptional.cpp -o testOptional -std=c++17
>  $ ./testOptional
>  a is engaged? 1
>  b is engaged? 1
>  
>  I would have expected:
>  a is engaged? 0
>  b is engaged? 1
> >>> I would have expected this too.

This is also what I would have expected.

>  This impacts the standard std::optional implementation on my machine as 
>  well as the local copy in WebKit’s wtf/Optional.h.
>  
>  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.
> >>> I believe it's UB to use an object after it has been moved.
> >> I'm wrong here.
> >> Apparently objects are left in a "valid but unspecified state".
> >> 
> >> https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove
> >>  
> >> 
> > I believe in WebKit, however, we’ve made sure our types are left in a valid 
> > “empty” state, thus my proposal for our own optional type that would be 
> > less error-prone / more convenient to use.
> I think your proposal is reasonable. I agree it's less error prone.
> 
> I think if we make this change, we should pull optional out of std and put it 
> in WTF, since we're now implementing behavior we will rely on being specified.

I am also in favor of having an implementation in WTF that empties the
optional after moving the value out from it.

Cheers,


-Adrián


pgpE_fxSDMoUF.pgp
Description: PGP signature
___
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

[Saam Barati]

> On Dec 14, 2018, at 1:59 PM, Chris Dumez  wrote:
> 
>> 
>> On Dec 14, 2018, at 1:56 PM, Saam Barati > > wrote:
>> 
>> 
>> 
>>> On Dec 14, 2018, at 1:54 PM, Saam Barati >> > wrote:
>>> 
>>> 
>>> 
 On Dec 14, 2018, at 1:37 PM, Chris Dumez >>> > wrote:
 
 Hi,
 
 I have now been caught twice by std::optional’s move constructor. It turns 
 out that it leaves the std::optional being moved-out *engaged*, it merely 
 moves its value.
 
 For example, testOptional.cpp:
 #include 
 #include 
 
 int main()
 {
 std::optional a = 1;
 std::optional b = std::move(a);
 std::cout << "a is engaged? " << !!a << std::endl;
 std::cout << "b is engaged? " << !!b << std::endl;
 return 0;
 }
 
 $ clang++ testOptional.cpp -o testOptional -std=c++17
 $ ./testOptional
 a is engaged? 1
 b is engaged? 1
 
 I would have expected:
 a is engaged? 0
 b is engaged? 1
>>> I would have expected this too.
>>> 
 
 This impacts the standard std::optional implementation on my machine as 
 well as the local copy in WebKit’s wtf/Optional.h.
 
 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.
>>> I believe it's UB to use an object after it has been moved.
>> I'm wrong here.
>> Apparently objects are left in a "valid but unspecified state".
>> 
>> https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove 
>> 
> I believe in WebKit, however, we’ve made sure our types are left in a valid 
> “empty” state, thus my proposal for our own optional type that would be less 
> error-prone / more convenient to use.
I think your proposal is reasonable. I agree it's less error prone.

I think if we make this change, we should pull optional out of std and put it 
in WTF, since we're now implementing behavior we will rely on being specified.

- Saam 

> 
>> 
>> - Saam
>>> 
>>> - Saam
>>> 
 As such, I find that std::optional’s move constructor behavior is 
 error-prone.
 
 I’d like to know how do other feel about this behavior? If enough people 
 agree this is error-prone, would we consider having our
 own optional type in WTF which resets the engaged flag (and never allow 
 the std::optional)?
 
 Thanks,
 --
  Chris Dumez
 
 
 
 
 ___
 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

[Chris Dumez]

> On Dec 14, 2018, at 1:56 PM, Saam Barati  wrote:
> 
> 
> 
>> On Dec 14, 2018, at 1:54 PM, Saam Barati > > wrote:
>> 
>> 
>> 
>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez >> > wrote:
>>> 
>>> Hi,
>>> 
>>> I have now been caught twice by std::optional’s move constructor. It turns 
>>> out that it leaves the std::optional being moved-out *engaged*, it merely 
>>> moves its value.
>>> 
>>> For example, testOptional.cpp:
>>> #include 
>>> #include 
>>> 
>>> int main()
>>> {
>>> std::optional a = 1;
>>> std::optional b = std::move(a);
>>> std::cout << "a is engaged? " << !!a << std::endl;
>>> std::cout << "b is engaged? " << !!b << std::endl;
>>> return 0;
>>> }
>>> 
>>> $ clang++ testOptional.cpp -o testOptional -std=c++17
>>> $ ./testOptional
>>> a is engaged? 1
>>> b is engaged? 1
>>> 
>>> I would have expected:
>>> a is engaged? 0
>>> b is engaged? 1
>> I would have expected this too.
>> 
>>> 
>>> This impacts the standard std::optional implementation on my machine as 
>>> well as the local copy in WebKit’s wtf/Optional.h.
>>> 
>>> 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.
>> I believe it's UB to use an object after it has been moved.
> I'm wrong here.
> Apparently objects are left in a "valid but unspecified state".
> 
> https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove 
> 
I believe in WebKit, however, we’ve made sure our types are left in a valid 
“empty” state, thus my proposal for our own optional type that would be less 
error-prone / more convenient to use.

> 
> - Saam
>> 
>> - Saam
>> 
>>> As such, I find that std::optional’s move constructor behavior is 
>>> error-prone.
>>> 
>>> I’d like to know how do other feel about this behavior? If enough people 
>>> agree this is error-prone, would we consider having our
>>> own optional type in WTF which resets the engaged flag (and never allow the 
>>> std::optional)?
>>> 
>>> Thanks,
>>> --
>>>  Chris Dumez
>>> 
>>> 
>>> 
>>> 
>>> ___
>>> 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

[Saam Barati]

> On Dec 14, 2018, at 1:54 PM, Saam Barati  wrote:
> 
> 
> 
>> On Dec 14, 2018, at 1:37 PM, Chris Dumez > > wrote:
>> 
>> Hi,
>> 
>> I have now been caught twice by std::optional’s move constructor. It turns 
>> out that it leaves the std::optional being moved-out *engaged*, it merely 
>> moves its value.
>> 
>> For example, testOptional.cpp:
>> #include 
>> #include 
>> 
>> int main()
>> {
>> std::optional a = 1;
>> std::optional b = std::move(a);
>> std::cout << "a is engaged? " << !!a << std::endl;
>> std::cout << "b is engaged? " << !!b << std::endl;
>> return 0;
>> }
>> 
>> $ clang++ testOptional.cpp -o testOptional -std=c++17
>> $ ./testOptional
>> a is engaged? 1
>> b is engaged? 1
>> 
>> I would have expected:
>> a is engaged? 0
>> b is engaged? 1
> I would have expected this too.
> 
>> 
>> This impacts the standard std::optional implementation on my machine as well 
>> as the local copy in WebKit’s wtf/Optional.h.
>> 
>> 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.
> I believe it's UB to use an object after it has been moved.
I'm wrong here.
Apparently objects are left in a "valid but unspecified state".

https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove 


- Saam
> 
> - Saam
> 
>> As such, I find that std::optional’s move constructor behavior is 
>> error-prone.
>> 
>> I’d like to know how do other feel about this behavior? If enough people 
>> agree this is error-prone, would we consider having our
>> own optional type in WTF which resets the engaged flag (and never allow the 
>> std::optional)?
>> 
>> Thanks,
>> --
>>  Chris Dumez
>> 
>> 
>> 
>> 
>> ___
>> 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

[Saam Barati]

> On Dec 14, 2018, at 1:37 PM, Chris Dumez  wrote:
> 
> Hi,
> 
> I have now been caught twice by std::optional’s move constructor. It turns 
> out that it leaves the std::optional being moved-out *engaged*, it merely 
> moves its value.
> 
> For example, testOptional.cpp:
> #include 
> #include 
> 
> int main()
> {
> std::optional a = 1;
> std::optional b = std::move(a);
> std::cout << "a is engaged? " << !!a << std::endl;
> std::cout << "b is engaged? " << !!b << std::endl;
> return 0;
> }
> 
> $ clang++ testOptional.cpp -o testOptional -std=c++17
> $ ./testOptional
> a is engaged? 1
> b is engaged? 1
> 
> I would have expected:
> a is engaged? 0
> b is engaged? 1
I would have expected this too.

> 
> This impacts the standard std::optional implementation on my machine as well 
> as the local copy in WebKit’s wtf/Optional.h.
> 
> 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.
I believe it's UB to use an object after it has been moved.

- Saam

> As such, I find that std::optional’s move constructor behavior is error-prone.
> 
> I’d like to know how do other feel about this behavior? If enough people 
> agree this is error-prone, would we consider having our
> own optional type in WTF which resets the engaged flag (and never allow the 
> std::optional)?
> 
> Thanks,
> --
>  Chris Dumez
> 
> 
> 
> 
> ___
> 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] Watch out for std::optional's move constructor

[Chris Dumez]

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out 
that it leaves the std::optional being moved-out *engaged*, it merely moves its 
value.

For example, testOptional.cpp:
#include 
#include 

int main()
{
std::optional a = 1;
std::optional b = std::move(a);
std::cout << "a is engaged? " << !!a << std::endl;
std::cout << "b is engaged? " << !!b << std::endl;
return 0;
}

$ clang++ testOptional.cpp -o testOptional -std=c++17
$ ./testOptional
a is engaged? 1
b is engaged? 1

I would have expected:
a is engaged? 0
b is engaged? 1

This impacts the standard std::optional implementation on my machine as well as 
the local copy in WebKit’s wtf/Optional.h.

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.
As such, I find that std::optional’s move constructor behavior is error-prone.

I’d like to know how do other feel about this behavior? If enough people agree 
this is error-prone, would we consider having our
own optional type in WTF which resets the engaged flag (and never allow the 
std::optional)?

Thanks,
--
 Chris Dumez




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