Bug 211674 – Switch from WTF::Optional to std::optional

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Switch from WTF::Optional to std::optional

[Darin Adler]

Created attachment 398944 [details]
Patch

[Darin Adler]

Comment on attachment 398944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398944&action=review

> Source/WTF/ChangeLog:3
> +        Switch from WTF::Optional to std::optional

I think a first cut would be to check the changes that are needed to get compiling with std::optional, without actually changing from WTF::Optional to std::optional.

Hoping EWS can find more of those. I found a number of them.

> Source/WTF/wtf/Optional.h:58
> +#define hasValue has_value
> +#define valueOr value_or

This super-horrible hack seems like something I would not check in.

[Chris Dumez]

The move constructor of std::optional behaves very differently iirc. We have to watch out everywhere we WTFMove() an optional. I thought we had decided to stick with our WTF class because of move constructor was safer. We have had multiple bugs due to use after move of std optional in the past. I believe there was a mailing list discussion about it.

[Darin Adler]

OK, that seems like a reason to not switch yet.

Are you saying we might never switch?

On the other hand, std::optional seems do to some other subtle things differently in a way that’s better. That led to the other changes in this patch.

[Darin Adler]

I like being safer, and if this benefit is great enough we should keep it. But in the long run I think it would be better if we were using standard C++ and not our own dialect.

[Chris Dumez]

I found the previous discussion on this:
https://lists.webkit.org/pipermail/webkit-dev/2018-December/030316.html

[Darin Adler]

How did you get experience this? Why were you using std::optional? Was it in another project?

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> I like being safer, and if this benefit is great enough we should keep it.
> But in the long run I think it would be better if we were using standard C++
> and not our own dialect.

I agree it is better to use standard types in general. However, the fact that the move constructor of std::optional doesn’t reset the isSet flag is a serious issue that has caught us several times :(

The proper pattern would be to use std::exchange() instead of WTFMove(). However, people are used to moving and I fear we will keep having such bugs if we made the switch.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> How did you get experience this? Why were you using std::optional? Was it in
> another project?

Definitely in WebKit. I have fixed several bugs caused by std optional move constructor behavior, which is why I decided to start that thread.

[Darin Adler]

Where in WebKit was std::optional used?

[Darin Adler]

(In reply to Chris Dumez from comment #8)
> The proper pattern would be to use std::exchange() instead of WTFMove().
> However, people are used to moving and I fear we will keep having such bugs
> if we made the switch.

Lets start moving from WTFMove to std::exchange. Some day we will make the switch.

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> Where in WebKit was std::optional used?

We used our own copy of std optional everywhere because we could not adopt the compiler one yet. We moved the WTF optional when I changed the move constructor if I remember correctly. Last I checked, which admittedly was at the time of that thread, the standard / compiler optional move constructor has the same behavior as our local copy of std optional.

[Chris Dumez]

I am kind of curious if the tests are going to pass. I do think people have been using std exchange more but I would not been surprised if more used after move cases of optional had been added.

[Darin Adler]

Looks like all the tests pass except for API tests that are depending on the move semantics. So it seems like we don’t have any code that depends on our different WTF::optional move semantics that affects any of our regression tests.

Separately, for some reason this didn’t compile on Windows, although it did on WinCairo. I can’t find an error message. I think I’ll push the "retry tests" button.

[Darin Adler]

Uploaded a patch fixing the problems found by using std::optional in bug 211703. Checking these in does *not* commit us to switching to std::optional, but will shrink this patch here.

[Darin Adler]

Created attachment 399101 [details]
Patch

[Darin Adler]

Created attachment 429548 [details]
Patch

[Darin Adler]

Created attachment 429565 [details]
Patch

[Darin Adler]

Created attachment 429590 [details]
Patch

[Darin Adler]

Created attachment 429608 [details]
Patch

[Darin Adler]

Created attachment 429657 [details]
Patch

[Darin Adler]

Trying to figure out what to do next:

- This Patch

This current patch seems to build and pass all the tests, so I could get it reviewed and landed as is.

- Smaller

I could make a smaller patch with all the changes that don’t require switching to std::optional (like I did a year back). That would include all the changes in JavaScriptCore, WebCore, and WebKit, the changes to Second.h and PersistentCoder.h in WTF, and the changes to the RetainPtr tests in TestWebKitAPI. That’s easy to do, and can be done without making any decision about adopting std::optional or deciding whether to use the macro tricks in Optional.h to smooth over the transition.

- Bigger

Could go for a bigger patch that does more global replacing. Likely the first step would be to remove the need for the macro tricks for hasValue and valueOr, then do the rest of the work towards removing the need for the Optional.h header entirely, using std::optional, std::make_unique, std::nullopt, std::nullopt_t, and getting rid of valueOrCompute.

[Chris Dumez]

(In reply to Darin Adler from comment #22)
> Trying to figure out what to do next:
> 
> - This Patch
> 
> This current patch seems to build and pass all the tests, so I could get it
> reviewed and landed as is.
> 
> - Smaller
> 
> I could make a smaller patch with all the changes that don’t require
> switching to std::optional (like I did a year back). That would include all
> the changes in JavaScriptCore, WebCore, and WebKit, the changes to Second.h
> and PersistentCoder.h in WTF, and the changes to the RetainPtr tests in
> TestWebKitAPI. That’s easy to do, and can be done without making any
> decision about adopting std::optional or deciding whether to use the macro
> tricks in Optional.h to smooth over the transition.
> 
> - Bigger
> 
> Could go for a bigger patch that does more global replacing. Likely the
> first step would be to remove the need for the macro tricks for hasValue and
> valueOr, then do the rest of the work towards removing the need for the
> Optional.h header entirely, using std::optional, std::make_unique,
> std::nullopt, std::nullopt_t, and getting rid of valueOrCompute.

I think we should land this patch as is. It is a good first step. Then we should probably follow-up to drop usage of WTF::Optional entirely?

[Chris Dumez]

Comment on attachment 429657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429657&action=review

r=me

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:310
> +    return options;

I would have updated early returns to return nullptr instead of { }. I assume it is equivalent but I am not 100% sure.

[Darin Adler]

Comment on attachment 429657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429657&action=review

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:310
>> +    return options;
> 
> I would have updated early returns to return nullptr instead of { }. I assume it is equivalent but I am not 100% sure.

Seems OK, but I prefer not to do it before landing because it’s not really different between optionals and pointers.

The caller is not supposed to look at the return value at all when there’s an exception, so I sort of like using { } rather than giving a specific return value. I kind of wish the RETURN_IF_EXCEPTION macro did that, but I guess it doesn’t work when the return type is void.

[Darin Adler]

Committed r278035 (238131@main): <https://commits.webkit.org/238131@main>

[Radar WebKit Bug Importer]

<rdar://problem/78470754>

[Lauro Moura]

Committed r278074 (238153@main): <https://commits.webkit.org/238153@main>

[Lauro Moura]

Committed r278076 (238155@main): <https://commits.webkit.org/238155@main>

[Darin Adler]

There’s no need for the extra braces in these GCC8 fixes. We would want you to write them like this:

    return SerializedPlatformDataCueValue { };