Summary: | Use default std::optional if it is provided | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, cdumez, clopez, cmarcelo, commit-queue, dbates, ews-watchlist, jbedard, jfbastien, magomez, mcatanzaro, saam, sam, 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
Yusuke Suzuki
2018-04-30 23:05:38 PDT
Created attachment 339186 [details]
Patch
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.
Created attachment 339187 [details]
Patch
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.
Created attachment 339188 [details]
Patch
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.
Created attachment 339194 [details]
Patch
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.
Created attachment 339197 [details]
Patch
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 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 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. 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 :) Created attachment 339261 [details]
Patch for landing
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.
Created attachment 339262 [details]
Patch for landing
Created attachment 339263 [details]
Patch for landing
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.
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.
Created attachment 339267 [details]
Patch for landing
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 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 :) (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 Committed r231223: <https://trac.webkit.org/changeset/231223> Follow-up build fix <https://trac.webkit.org/changeset/231251>. (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>. Re-opened since this is blocked by bug 185229 (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>. (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. 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 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. 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; ^~~~~~~~ (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. 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 ^~~~~~~~~ *** Bug 185194 has been marked as a duplicate of this bug. *** 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? (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. *** Bug 185198 has been marked as a duplicate of this bug. *** Created attachment 339489 [details]
Patch
(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. (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 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 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>. Created attachment 339491 [details]
Patch
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. 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 on attachment 339491 [details]
Patch
Clang build is still broken for me, but the error is no longer related to std::optional. r=me
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 :) Thanks. After EWS becomes green, I'll land this :) OK, mac becomes green, landing. Committed r231342: <https://trac.webkit.org/changeset/231342> |