Bug 185159 – Use default std::optional if it is provided

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Use default std::optional if it is provided

[Yusuke Suzuki]

Created attachment 339186 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 339187 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 339188 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 339194 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 339197 [details]
Patch

[EWS Watchlist]

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.

[JF Bastien]

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

[Alex Christensen]

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.

[JF Bastien]

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 :)

[Yusuke Suzuki]

Created attachment 339261 [details]
Patch for landing

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 339262 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 339263 [details]
Patch for landing

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 339267 [details]
Patch for landing

[EWS Watchlist]

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.

[Yusuke Suzuki]

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 :)

[Yusuke Suzuki]

(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

[Yusuke Suzuki]

Committed r231223: <https://trac.webkit.org/changeset/231223>

[Radar WebKit Bug Importer]

<rdar://problem/39891074>

[Jonathan Bedard]

Follow-up build fix <https://trac.webkit.org/changeset/231251>.

[Yusuke Suzuki]

(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>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185229

[Yusuke Suzuki]

(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>.

[Jonathan Bedard]

(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.

[Jonathan Bedard]

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

[Yusuke Suzuki]

(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.

[Michael Catanzaro]

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;
                                                                                                     ^~~~~~~~

[Yusuke Suzuki]

(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.

[Michael Catanzaro]

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
          ^~~~~~~~~

[Michael Catanzaro]

*** Bug 185194 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

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?

[Yusuke Suzuki]

(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.

[Michael Catanzaro]

*** Bug 185198 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 339489 [details]
Patch

[Michael Catanzaro]

(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.

[Yusuke Suzuki]

(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

[Michael Catanzaro]

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.

[Yusuke Suzuki]

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>.

[Yusuke Suzuki]

Created attachment 339491 [details]
Patch

[Michael Catanzaro]

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.

[EWS Watchlist]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

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 :)

[Yusuke Suzuki]

Thanks. After EWS becomes green, I'll land this :)

[Yusuke Suzuki]

OK, mac becomes green, landing.

[Yusuke Suzuki]

Committed r231342: <https://trac.webkit.org/changeset/231342>