Bug 185159

Summary: Use default std::optional if it is provided
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, clopez, cmarcelo, commit-queue, dbates, ews-watchlist, jbedard, jfbastien, magomez, mcatanzaro, sam, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185176
https://bugs.webkit.org/show_bug.cgi?id=187669
https://bugs.webkit.org/show_bug.cgi?id=187672
Bug Depends on: 185229, 185256    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch
none
Patch mcatanzaro: review+

Description Yusuke Suzuki 2018-04-30 23:05:38 PDT
Use default std::optional if it is provided
Comment 1 Yusuke Suzuki 2018-04-30 23:33:05 PDT
Created attachment 339186 [details]
Patch
Comment 2 EWS Watchlist 2018-04-30 23:34:29 PDT
Attachment 339186 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Yusuke Suzuki 2018-04-30 23:45:28 PDT
Created attachment 339187 [details]
Patch
Comment 4 EWS Watchlist 2018-04-30 23:47:43 PDT
Attachment 339187 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Yusuke Suzuki 2018-04-30 23:49:25 PDT
Created attachment 339188 [details]
Patch
Comment 6 EWS Watchlist 2018-04-30 23:52:50 PDT
Attachment 339188 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2018-05-01 01:07:34 PDT
Created attachment 339194 [details]
Patch
Comment 8 EWS Watchlist 2018-05-01 01:09:07 PDT
Attachment 339194 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Yusuke Suzuki 2018-05-01 08:18:44 PDT
Created attachment 339197 [details]
Patch
Comment 10 EWS Watchlist 2018-05-01 08:19:56 PDT
Attachment 339197 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 JF Bastien 2018-05-01 08:51:26 PDT
Comment on attachment 339197 [details]
Patch

Hmm the segfaults are annoying :(
I want to make sure we don't break Linux builds. Can you confirm that it works locally for you?

I started a patch yesterday to turn C++17 on in the Xcode build settings, will finish it later today (have a meeting now...). I think landing your patch first would be good because it'll simplify what I need to do, so assuming bots are happy r=me
Comment 12 Alex Christensen 2018-05-01 10:44:51 PDT
Comment on attachment 339197 [details]
Patch

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

> Source/WTF/ChangeLog:15
> +        And we also remove std::optional<T&> use since this is not accepted in C++17. Use
> +        std::optional<std::reference_wrapper<T>> instead.

We often use T* for this purpose.
Comment 13 JF Bastien 2018-05-01 17:08:49 PDT
This patch might be subsumed by https://bugs.webkit.org/show_bug.cgi?id=185176
It can get checked in before as well, either works :)
Comment 14 Yusuke Suzuki 2018-05-01 20:14:43 PDT
Created attachment 339261 [details]
Patch for landing
Comment 15 EWS Watchlist 2018-05-01 20:15:47 PDT
Attachment 339261 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Yusuke Suzuki 2018-05-01 20:24:26 PDT
Created attachment 339262 [details]
Patch for landing
Comment 17 Yusuke Suzuki 2018-05-01 20:25:29 PDT
Created attachment 339263 [details]
Patch for landing
Comment 18 EWS Watchlist 2018-05-01 20:27:18 PDT
Attachment 339262 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 EWS Watchlist 2018-05-01 20:28:28 PDT
Attachment 339263 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Yusuke Suzuki 2018-05-01 21:06:35 PDT
Created attachment 339267 [details]
Patch for landing
Comment 21 EWS Watchlist 2018-05-01 21:09:18 PDT
Attachment 339267 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1061:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Yusuke Suzuki 2018-05-01 21:12:22 PDT
Comment on attachment 339197 [details]
Patch

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

>> Source/WTF/ChangeLog:15
>> +        std::optional<std::reference_wrapper<T>> instead.
> 
> We often use T* for this purpose.

I would like to change this in a separate patch :)
Comment 23 Yusuke Suzuki 2018-05-01 21:32:13 PDT
(In reply to JF Bastien from comment #11)
> Comment on attachment 339197 [details]
> Patch
> 
> Hmm the segfaults are annoying :(
> I want to make sure we don't break Linux builds. Can you confirm that it
> works locally for you?

Yeah, finally, it works :)

> 
> I started a patch yesterday to turn C++17 on in the Xcode build settings,
> will finish it later today (have a meeting now...). I think landing your
> patch first would be good because it'll simplify what I need to do, so
> assuming bots are happy r=me
Comment 24 Yusuke Suzuki 2018-05-01 21:32:40 PDT
Committed r231223: <https://trac.webkit.org/changeset/231223>
Comment 25 Radar WebKit Bug Importer 2018-05-01 21:33:27 PDT
<rdar://problem/39891074>
Comment 26 Jonathan Bedard 2018-05-02 11:56:01 PDT
Follow-up build fix <https://trac.webkit.org/changeset/231251>.
Comment 27 Yusuke Suzuki 2018-05-02 17:24:06 PDT
(In reply to Jonathan Bedard from comment #26)
> Follow-up build fix <https://trac.webkit.org/changeset/231251>.

This breaks GCC 7.3 build since __cpp_lib_optional is defined in <optional>.
Comment 28 WebKit Commit Bot 2018-05-02 17:26:41 PDT
Re-opened since this is blocked by bug 185229
Comment 29 Yusuke Suzuki 2018-05-02 17:30:05 PDT
(In reply to Yusuke Suzuki from comment #27)
> (In reply to Jonathan Bedard from comment #26)
> > Follow-up build fix <https://trac.webkit.org/changeset/231251>.
> 
> This breaks GCC 7.3 build since __cpp_lib_optional is defined in <optional>.

Jonathan, can we have the different fix for this?(In reply to Jonathan Bedard from comment #26)
> Follow-up build fix <https://trac.webkit.org/changeset/231251>.

Jonathan, can we have a different fix for this? Your previous fix, checking "__cpp_lib_optional" before including <optional>, does not work since "__cpp_lib_optional" is defined in <optional>.
Comment 30 Jonathan Bedard 2018-05-03 08:13:58 PDT
(In reply to Yusuke Suzuki from comment #29)
> (In reply to Yusuke Suzuki from comment #27)
> > (In reply to Jonathan Bedard from comment #26)
> > > Follow-up build fix <https://trac.webkit.org/changeset/231251>.
> > 
> > This breaks GCC 7.3 build since __cpp_lib_optional is defined in <optional>.
> 
> Jonathan, can we have the different fix for this?(In reply to Jonathan
> Bedard from comment #26)
> > Follow-up build fix <https://trac.webkit.org/changeset/231251>.
> 
> Jonathan, can we have a different fix for this? Your previous fix, checking
> "__cpp_lib_optional" before including <optional>, does not work since
> "__cpp_lib_optional" is defined in <optional>.

Oops!  You're right, fixed one build, broke another.
Comment 31 Jonathan Bedard 2018-05-03 09:59:17 PDT
Rolled out in r231308 because this is breaking most of our builds.

Where is this '__cpp_lib_optional' macro coming from? It doesn't seem to be defined in the <optional> header I'm looking at.

The compiler failure we're hitting is this:

.../WebKitBuild/Debug/usr/local/include/wtf/Optional.h:297:7: error: redefinition of 'bad_optional_access'
class bad_optional_access : public std::logic_error {

....

.../usr/include/c++/v1/optional:167:29: note: previous definition is here
class _LIBCPP_EXCEPTION_ABI bad_optional_access
Comment 32 Yusuke Suzuki 2018-05-03 16:19:30 PDT
(In reply to Jonathan Bedard from comment #31)
> Rolled out in r231308 because this is breaking most of our builds.
> 
> Where is this '__cpp_lib_optional' macro coming from? It doesn't seem to be
> defined in the <optional> header I'm looking at.
> 
> The compiler failure we're hitting is this:
> 
> .../WebKitBuild/Debug/usr/local/include/wtf/Optional.h:297:7: error:
> redefinition of 'bad_optional_access'
> class bad_optional_access : public std::logic_error {
> 
> ....
> 
> .../usr/include/c++/v1/optional:167:29: note: previous definition is here
> class _LIBCPP_EXCEPTION_ABI bad_optional_access

In libstdc++ (GCC), https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/optional#L54 defines this macro.
Could you check your environment's configuration? Since I don't have that environment and EWS says green, I cannot check how to avoid this problem.
Comment 33 Michael Catanzaro 2018-05-03 16:29:25 PDT
Um, this is weird, but the *rollout* of this commit seems to have broken the GCC 8 build.

In file included from ../../Source/WTF/wtf/Threading.h:37,
                 from ../../Source/WTF/wtf/AutomaticThread.cpp:30:
../../Source/WTF/wtf/Expected.h:576:101: error: binding reference of type ‘std::experimental::fundamentals_v3::unexpected_t&’ to ‘const std::experimental::fundamentals_v3::unexpected_t’ discards qualifiers
 __EXPECTED_INLINE_VARIABLE constexpr std::experimental::unexpected_t& unexpect = std::experimental::unexpect;
                                                                                                     ^~~~~~~~
Comment 34 Yusuke Suzuki 2018-05-03 16:34:19 PDT
(In reply to Michael Catanzaro from comment #33)
> Um, this is weird, but the *rollout* of this commit seems to have broken the
> GCC 8 build.
> 
> In file included from ../../Source/WTF/wtf/Threading.h:37,
>                  from ../../Source/WTF/wtf/AutomaticThread.cpp:30:
> ../../Source/WTF/wtf/Expected.h:576:101: error: binding reference of type
> ‘std::experimental::fundamentals_v3::unexpected_t&’ to ‘const
> std::experimental::fundamentals_v3::unexpected_t’ discards qualifiers
>  __EXPECTED_INLINE_VARIABLE constexpr std::experimental::unexpected_t&
> unexpect = std::experimental::unexpect;
>                                                                             
> ^~~~~~~~

Yeah, currently, GCC 7.3 build is broken too.
Comment 35 Michael Catanzaro 2018-05-03 16:39:53 PDT
Also:

In file included from /usr/include/c++/8/bits/node_handle.h:39,
                 from /usr/include/c++/8/bits/hashtable.h:37,
                 from /usr/include/c++/8/unordered_map:46,
                 from /usr/include/c++/8/functional:61,
                 from ../../Source/WTF/wtf/Variant.h:42,
                 from ../../Source/WTF/wtf/text/TextBreakIterator.h:26,
                 from ../../Source/WTF/wtf/text/StringView.cpp:36:
/usr/include/c++/8/optional:60:10: error: redefinition of ‘struct std::nullopt_t’
   struct nullopt_t
          ^~~~~~~~~
Comment 36 Michael Catanzaro 2018-05-03 16:40:38 PDT
*** Bug 185194 has been marked as a duplicate of this bug. ***
Comment 37 Michael Catanzaro 2018-05-03 16:54:00 PDT
Yusuke, here is the libc++ header:

https://llvm.org/viewvc/llvm-project/libcxx/trunk/include/optional?view=markup

Note there's no __cpp_lib_optional.

Isn't it sufficient to check && __has_include(<optional>) instead?
Comment 38 Yusuke Suzuki 2018-05-03 16:58:27 PDT
(In reply to Michael Catanzaro from comment #37)
> Yusuke, here is the libc++ header:
> 
> https://llvm.org/viewvc/llvm-project/libcxx/trunk/include/
> optional?view=markup
> 
> Note there's no __cpp_lib_optional.
> 
> Isn't it sufficient to check && __has_include(<optional>) instead?

Discussed with jbedard at IRC.
In the internal environment, we do not want to include <optional>. But <optional> header exists.
So, COMPILER(CLANG) and version check would work. But I also want to check whether this works in Debian stable's clang bot.
clopez, could you take a look? I'll upload a merged patch soon.
Comment 39 Michael Catanzaro 2018-05-03 17:01:35 PDT
*** Bug 185198 has been marked as a duplicate of this bug. ***
Comment 40 Yusuke Suzuki 2018-05-03 17:03:46 PDT
Created attachment 339489 [details]
Patch
Comment 41 Michael Catanzaro 2018-05-03 17:04:08 PDT
(In reply to Yusuke Suzuki from comment #38)
> So, COMPILER(CLANG) and version check would work. But I also want to check
> whether this works in Debian stable's clang bot.
> clopez, could you take a look? I'll upload a merged patch soon.

That bot is currently using libstdc++ 4.9. It's just not going to be practical to get that working with C++ 17. clopez is planning to upgrade the bot. Let's not worry about it for now.
Comment 42 Yusuke Suzuki 2018-05-03 17:07:51 PDT
(In reply to Michael Catanzaro from comment #41)
> (In reply to Yusuke Suzuki from comment #38)
> > So, COMPILER(CLANG) and version check would work. But I also want to check
> > whether this works in Debian stable's clang bot.
> > clopez, could you take a look? I'll upload a merged patch soon.
> 
> That bot is currently using libstdc++ 4.9. It's just not going to be
> practical to get that working with C++ 17. clopez is planning to upgrade the
> bot. Let's not worry about it for now.

OK nice. So, if the uploaded patch works, everything is OK :D
Comment 43 Michael Catanzaro 2018-05-03 17:08:00 PDT
Comment on attachment 339489 [details]
Patch

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

> Source/WTF/wtf/Optional.h:54
> +#if !COMPILER(MSVC) && !COMPILER(CLANG) && __has_include(<optional>)
> +# include <optional>
> +#endif
> +
> +#if !COMPILER(MSVC) && !COMPILER(CLANG) && defined(__cpp_lib_optional) && __cpp_lib_optional >= 201603

I hesitate to r- a patch that's going to fix the build for me personally, but this is still going to be broken with Clang and that's not OK.
Comment 44 Yusuke Suzuki 2018-05-03 17:10:51 PDT
Comment on attachment 339489 [details]
Patch

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

>> Source/WTF/wtf/Optional.h:54
>> +#if !COMPILER(MSVC) && !COMPILER(CLANG) && defined(__cpp_lib_optional) && __cpp_lib_optional >= 201603
> 
> I hesitate to r- a patch that's going to fix the build for me personally, but this is still going to be broken with Clang and that's not OK.

I'll change this to use `PLATFORM(COCOA)` instead. Basically, clang should work if we can include <optional>.
Comment 45 Yusuke Suzuki 2018-05-03 17:12:41 PDT
Created attachment 339491 [details]
Patch
Comment 46 Michael Catanzaro 2018-05-03 17:12:52 PDT
I tested your patch. It works with GCC. But, as expected, I see tons of errors with clang, related to nullopt_t.

(In reply to Yusuke Suzuki from comment #44)
> I'll change this to use `PLATFORM(COCOA)` instead. Basically, clang should
> work if we can include <optional>.

That's not great either, but I guess PLATFORM(COCOA) is good enough for now, OK. r=me, then.
Comment 47 EWS Watchlist 2018-05-03 17:13:53 PDT
Attachment 339491 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:1055:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Michael Catanzaro 2018-05-03 17:14:20 PDT
Comment on attachment 339491 [details]
Patch

Clang build is still broken for me, but the error is no longer related to std::optional. r=me
Comment 49 Michael Catanzaro 2018-05-03 17:16:04 PDT
GCC build is (probably) fixed (at least the build did not fail in first 30 seconds, so if it's still broken it won't be due to std::optional :)
Comment 50 Yusuke Suzuki 2018-05-03 17:20:45 PDT
Thanks. After EWS becomes green, I'll land this :)
Comment 51 Yusuke Suzuki 2018-05-03 17:55:44 PDT
OK, mac becomes green, landing.
Comment 52 Yusuke Suzuki 2018-05-03 18:00:23 PDT
Committed r231342: <https://trac.webkit.org/changeset/231342>