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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

Again, the C++ standard does not say that moving from an object leaves the object in an undefined state. The C++ standard does say: > Objects of types defined in the C++ standard library may be moved from > (12.8). Move operations may be explicitly specified or implicitly generated. > Unless

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

We can definitely make our own WTF::Optional instead of using std::optional and change its move constructor/operator= and I personally think that would not be worth it but if I’m in the minority I’ll deal with it. We cannot diverge from the C++ standards that say that moving from an object

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

Let me add this. The situation we have here is analogous to having a member function "move()" which leave std::optional in undefined state as in: std::optional a; std::optional b = a.move(); would leave a in some undefined state. I don't care what C++ standards say or what STL does. That's

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

It's true that WTFMove or std::move doesn't do anything if the moved variable is not used because WTFMove / std::move is just a type cast. However, that behavior is orthogonal from the issue that calling WTFMove / std::move on std::optional, and the returned value is assigned to another

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

> On Dec 17, 2018, at 3:55 PM, Alex Christensen wrote: > > Let me give a concrete example on why, even with our nice-to-use WTF types, > the state of a C++ object is undefined after being moved from: > > #include > #include > #include > > class Test : public RefCounted { }; > > void

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

Let me give a concrete example on why, even with our nice-to-use WTF types, the state of a C++ object is undefined after being moved from: #include #include #include class Test : public RefCounted { }; void useParameter(RefPtr&& param) { RefPtr usedParam = WTFMove(param); } void

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

Yeah, it seems like making std::optional more in line with our own convention provides more merits than downsides here. People are using WTFMove as if it's some sort of a swap operation in our codebase, and as Maciej pointed out, having rules where people have to think carefully as to when & when

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

I don’t understand the claim about “undefined behavior” here. As Maciej pointed out, these are our libraries. We are free to define their behaviors. In general, “undefined behavior” is an unwanted feature of programming languages and libraries, which we accept begrudgingly simply because there

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

This one and the many others like it are fragile, relying on undefined behavior, and should be replaced by std::exchange. Such a change was made in https://trac.webkit.org/changeset/198755/webkit and we probably need many more like that, but we are getting away with relying on undefined

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

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

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

> On Dec 17, 2018, at 10:27 AM, Alex Christensen wrote: > > On Dec 14, 2018, at 1:37 PM, Chris Dumez > wrote: > > As far as I know, our convention in WebKit so far for our types has been > that types getting moved-out are left in a valid

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

On Dec 14, 2018, at 1:37 PM, Chris Dumez >>> > wrote: >>> As far as I know, our convention in WebKit so far for our types has been that types getting moved-out are left in a valid “empty” state. This is not necessarily true. When we move out of an

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

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

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

> 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

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

> 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

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

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

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

> 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

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

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?

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

> 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> 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

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

> 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

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

> 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

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

> 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