Switch from WTF::Optional to std::optional
Created attachment 398944 [details] Patch
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.
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.
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.
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 found the previous discussion on this: https://lists.webkit.org/pipermail/webkit-dev/2018-December/030316.html
How did you get experience this? Why were you using std::optional? Was it in another project?
(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.
(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.
Where in WebKit was std::optional used?
(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.
(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.
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.
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.
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.
Created attachment 399101 [details] Patch
Created attachment 429548 [details] Patch
Created attachment 429565 [details] Patch
Created attachment 429590 [details] Patch
Created attachment 429608 [details] Patch
Created attachment 429657 [details] Patch
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.
(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?
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.
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.
Committed r278035 (238131@main): <https://commits.webkit.org/238131@main>
<rdar://problem/78470754>
Committed r278074 (238153@main): <https://commits.webkit.org/238153@main>
Committed r278076 (238155@main): <https://commits.webkit.org/238155@main>
There’s no need for the extra braces in these GCC8 fixes. We would want you to write them like this: return SerializedPlatformDataCueValue { };