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

[Maciej Stachowiak]

> On Dec 16, 2018, at 3:47 PM, Daniel Bates <dbates at webkit.org> wrote:
> 
> 
> 
>> On Dec 14, 2018, at 3:52 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> 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 <https://trac.webkit.org/changeset/239228 <https://trac.webkit.org/changeset/239228>>.

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 <http://en.cppreference.com/w/cpp/container/vector><std::string <http://en.cppreference.com/w/cpp/string/basic_string>> v;
> std::string <http://en.cppreference.com/w/cpp/string/basic_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
> 
> ]]
> <https://en.cppreference.com/w/cpp/utility/move <https://en.cppreference.com/w/cpp/utility/move>>
> 
> (*) 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 <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:
>>> 
>>> 
>>>> On Dec 14, 2018, at 3:39 PM, Fujii Hironori <fujii.hironori at gmail.com <mailto:fujii.hironori at gmail.com>> wrote:
>>>> 
>>>> 
>>>> On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> 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 <https://trac.webkit.org/changeset/239228/webkit>
>>> 
>>> 
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev>
>> 
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev>_______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20181216/964fdab6/attachment.html>