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