Bug 185159 - Use default std::optional if it is provided
Summary: Use default std::optional if it is provided
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 185229 185256
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-30 23:05 PDT by Yusuke Suzuki
Modified: 2018-07-13 16:59 PDT (History)
15 users (show)

See Also:


Attachments
Patch (16.12 KB, patch)
2018-04-30 23:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.27 KB, patch)
2018-04-30 23:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.29 KB, patch)
2018-04-30 23:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.26 KB, patch)
2018-05-01 01:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (21.09 KB, patch)
2018-05-01 08:18 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (22.32 KB, patch)
2018-05-01 20:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (23.07 KB, patch)
2018-05-01 20:24 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (22.98 KB, patch)
2018-05-01 20:25 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (21.20 KB, patch)
2018-05-01 21:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2018-05-03 17:03 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2018-05-03 17:12 PDT, Yusuke Suzuki
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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>