Use designated initializers #24531

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2203-designated_init-🛍 changing 4 files +38 −7
  1. maflcko commented at 1:13 pm on March 11, 2022: member
    Designated initializers are supported since gcc 4.7 (Our minimum required is 8) and clang 3 (Our minimum required is 7). They work out of the box with C++17, and only msvc requires the C++20 flag to be set. I don’t expect any of our msvc users will run into issues due to this. See also https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2022/bitcoin-core-dev.2022-03-10-19.00.log.html#l-114
  2. maflcko commented at 1:13 pm on March 11, 2022: member
    Looks like there is a msvc bug that causes fatal error C1060: compiler is out of heap space :man_shrugging:
  3. jonatack commented at 1:55 pm on March 11, 2022: contributor 
    0C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(687,5): error MSB8071: Cannot parse tool output 'C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\rpc\blockchain.cpp(968,1): fatal error C1060: compiler is out of heap space' with regex '^\s*(?<FILENAME>((.:)?[^:\n\r]*?))(\((?<LINE>[0-9]*)(,(?<COLUMN>[0-9]*))?\))?\s*:\s*((?<SUBCATEGORY>([^:]*?))\s?)(?<CATEGORY>error|warning|note)\s*((?<CODE>[A-Za-z0-9]*))?\s*:\s*(?<TEXT>.*)$': Exception of type 'System.OutOfMemoryException' was thrown. [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]
1C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\type_traits(1278): fatal error C1060: compiler is out of heap space [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
2C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\vector(572,1): fatal error C1060: compiler is out of heap space [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
  4. DrahtBot added the label P2P on Mar 11, 2022
  5. in src/fs.h:60 in c4230b9c34 outdated 
    55+    {
56+        auto utf8_str{std::filesystem::path::u8string()};
57+        // utf8_str might either be std::string (C++17) or std::u8string
58+        // (C++20). Convert both to std::string. This method can be removed
59+        // after switching to C++20.
60+        return *(std::string*)(&utf8_str);
    luke-jr commented at 0:51 am on March 17, 2022:
    Is this well-defined behaviour?
    maflcko commented at 7:13 am on March 17, 2022:
    Practically probably yes. Theoretically no. I’ve pushed an alternative.
  6. in src/scheduler.cpp:114 in c4230b9c34 outdated 
    110@@ -111,7 +111,8 @@ static void Repeat(CScheduler& s, CScheduler::Function f, std::chrono::milliseco
111 
112 void CScheduler::scheduleEvery(CScheduler::Function f, std::chrono::milliseconds delta)
113 {
114-    scheduleFromNow([=] { Repeat(*this, f, delta); }, delta);
115+    auto* self{this}; // Avoid warning: implicit capture of ‘this’ via ‘[=]’ is deprecated in C++20 [-Wdeprecated]
    luke-jr commented at 0:52 am on March 17, 2022:
    Why not just make it explicit?
    maflcko commented at 6:05 am on March 17, 2022:

    Because it wouldn’t compile with C++17:

    0    [=, this] {};   // until C++20: Error: this when = is the default

    See also https://en.cppreference.com/w/cpp/language/lambda#Lambda_capture

    ajtowns commented at 2:26 pm on March 17, 2022:
    Being fully explicit should work? [this, f, delta] { Repeat(*this, f, delta; }
  7. maflcko force-pushed on Mar 17, 2022
  8. maflcko force-pushed on Mar 17, 2022
  9. in src/fs.h:60 in fab229109c outdated 
    55+    {
56+        const auto utf8_str{std::filesystem::path::u8string()};
57+        // utf8_str might either be std::string (C++17) or std::u8string
58+        // (C++20). Convert both to std::string. This method can be removed
59+        // after switching to C++20.
60+        return std::string{reinterpret_cast<const char*>(utf8_str.data()), utf8_str.size()};
    ajtowns commented at 3:06 pm on March 17, 2022:
    Should be able to just use return std::string(utf8_str.begin(), utf8_str.end()); here? See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r2.html
    maflcko commented at 4:20 pm on March 17, 2022:
    Right. I confused myself because the std::string_view{begin, end} constructor is missing from C++17, but the std::string one exists.
  10. maflcko force-pushed on Mar 17, 2022
  11. maflcko force-pushed on Mar 17, 2022
  12. maflcko commented at 4:55 pm on March 17, 2022: member
    @sipsorcery Any idea on how to approach the msvc bug?
  13. sipsorcery commented at 7:44 pm on March 17, 2022: member

    @sipsorcery Any idea on how to approach the msvc bug?

    I can’t replicate this bug locally. The changes in this PR build fine for me with msbuild and Visual Studio 2022. Could it be that the cirrus CI VM is out of memory? Is there an option to give it a bit more RAM to see if that fixes it?

  14. hebasto commented at 7:52 pm on March 17, 2022: member

    The changes in this PR build fine for me with msbuild and Visual Studio 2022.

    On Cirrus we have Visual Studio 2019:

    0> msbuild -version 
1Microsoft (R) Build Engine version 16.11.2+f32259642 for .NET Framework
2Copyright (C) Microsoft Corporation. All rights reserved.
316.11.2.50704
  15. maflcko commented at 8:00 pm on March 17, 2022: member
    Maybe we should bump it? Not sure how, though.
  16. sipsorcery commented at 8:58 am on March 18, 2022: member

    Looks like it was a memory issue on the Cirrus VM then?

    I did build this PR locally using Visual Studio 2019, with a version close to what Cirrus is using, and did not get any compiler errors. I got some linker errors but they are related to vcpkg dependencies being built with a newer Visual Studio version. I didn’t see any errors that looked like they were caused by this PR.

  17. maflcko marked this as ready for review on Mar 18, 2022
  18. maflcko commented at 9:01 am on March 18, 2022: member

    The change in memory requirements (spike) seems just a bit absurd:

    Screenshot 2022-03-18 at 09-59-31 Cirrus CI

  19. maflcko force-pushed on Mar 18, 2022
  20. maflcko force-pushed on Mar 18, 2022
  21. hebasto commented at 10:24 am on March 18, 2022: member
    CI is green now :smile:
  22. maflcko commented at 10:40 am on March 18, 2022: member
    Taken out of draft for review.
  23. DrahtBot commented at 5:46 pm on March 19, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  24. maflcko marked this as a draft on Mar 22, 2022
  25. maflcko force-pushed on Mar 24, 2022
  26. maflcko marked this as ready for review on Mar 24, 2022
  27. maflcko force-pushed on Mar 24, 2022
  28. maflcko commented at 6:33 am on March 25, 2022: member

    Looks like another msvc bug??

     0...
 1 node0 2022-03-24T22:07:43.249545Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\rpc\request.cpp:179] [parse] ThreadRPCServer method=addpeeraddress user=__cookie__ 
 2 node0 2022-03-24T22:07:43.249707Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\addrman.cpp:613] [AddSingle] Added 32.100.1.1:8333 mapped to AS0 to new[865][20] 
 3 node0 2022-03-24T22:07:43.249763Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\addrman.cpp:684] [Add_] Added 1 addresses (of 1) from 32.100.1.1: 0 tried, 7344 new 
 4 test  2022-03-24T22:07:43.264000Z TestFramework (ERROR): Unexpected exception caught during testing 
 5                                   Traceback (most recent call last):
 6                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
 7                                       self.run_test()
 8                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\p2p_getaddr_caching.py", line 52, in run_test
 9                                       self.nodes[0].addpeeraddress(a, 8333)
10                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\coverage.py", line 49, in __call__
11                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
12                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 142, in __call__
13                                       response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
14                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 107, in _request
15                                       self.__conn.request(method, path, postdata, headers)
16                                     File "C:\Python39\lib\http\client.py", line 1257, in request
17                                       self._send_request(method, url, body, headers, encode_chunked)
18                                     File "C:\Python39\lib\http\client.py", line 1303, in _send_request
19                                       self.endheaders(body, encode_chunked=encode_chunked)
20                                     File "C:\Python39\lib\http\client.py", line 1252, in endheaders
21                                       self._send_output(message_body, encode_chunked=encode_chunked)
22                                     File "C:\Python39\lib\http\client.py", line 1012, in _send_output
23                                       self.send(msg)
24                                     File "C:\Python39\lib\http\client.py", line 952, in send
25                                       self.connect()
26                                     File "C:\Python39\lib\http\client.py", line 925, in connect
27                                       self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
28                                   OSError: [WinError 10022] An invalid argument was supplied
29 test  2022-03-24T22:07:43.264000Z TestFramework (DEBUG): Closing down network thread 
30 node0 2022-03-24T22:07:43.265009Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\httpserver.cpp:242] [http_request_cb] Received a POST request for / from 127.0.0.1:20146 
31 node0 2022-03-24T22:07:43.265215Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\rpc\request.cpp:179] [parse] ThreadRPCServer method=addpeeraddress user=__cookie__ 
32 test  2022-03-24T22:07:43.327000Z TestFramework (INFO): Stopping nodes 
33 test  2022-03-24T22:07:43.327000Z TestFramework.node0 (DEBUG): Stopping node 
34 node0 2022-03-24T22:07:43.343120Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\httpserver.cpp:242] [http_request_cb] Received a POST request for / from 127.0.0.1:20155 
35 node0 2022-03-24T22:07:43.343329Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\rpc\request.cpp:179] [parse] ThreadRPCServer method=stop user=__cookie__ 
36...
  29. ajtowns commented at 2:29 pm on March 25, 2022: contributor
    That’s an enormous memory increase; seems to me like we’re better of putting this back in the “when we switch to C++20 fully” bucket? If we get CI catching any clang-tidy errors that’s 80% of the way there…
  30. maflcko commented at 2:37 pm on March 25, 2022: member
    We can also check if the bugs are fixed in Visual Studio 2022 and then bump to that. Looking at the recently reported issues for msvc builds, everyone seems to be using VS 2022 already. If not, it wouldn’t be hard for a dev/user to update to that either.
  31. maflcko marked this as a draft on Mar 25, 2022
  32. DrahtBot added the label Needs rebase on Mar 25, 2022
  33. ajtowns commented at 1:53 am on March 26, 2022: contributor

    We can also check if the bugs are fixed in Visual Studio 2022 and then bump to that. Looking at the recently reported issues for msvc builds, everyone seems to be using VS 2022 already. If not, it wouldn’t be hard for a dev/user to update to that either.

    Maybe setup a CI job for VS2022 and run that against master for a couple of months first? Just seems a bit bleeding edge for a feature that’s really only a nice-to-have and that we have a workable alternative for.

  34. sipsorcery commented at 5:06 pm on March 26, 2022: member

    I get the same functional test failure with p2p_getaddr_caching with and without this PR. I’ve actually never had much luck getting the functional tests to succeed on Windows.

    I would find it strange that named initialisers could result in that sort of failure. My understanding is they are syntatic sugar and if they compile correctly then the output should be equivalent to non-named initialisers. Excepting any compiler bugs but again that seems unlikely in this situation.

    This PR and master build susccessfully for me with VS 2022 using:

    msvc-autogen.py -toolset v143

    IMHO switching the CI to VS 2022 is a reasonable course of action. The msvc build is meant for testing, learning and early warning purposes. Arguably those are best achieved by using the latest stable version of the msvc toolchain.

  35. maflcko force-pushed on Mar 28, 2022
  36. maflcko commented at 12:26 pm on March 28, 2022: member
    Rebased. I’ll leave this in draft to wait a bit more on https://github.com/cirruslabs/cirrus-ci-docs/issues/985
  37. maflcko force-pushed on Mar 28, 2022
  38. maflcko force-pushed on Mar 28, 2022
  39. DrahtBot removed the label Needs rebase on Mar 28, 2022
  40. maflcko marked this as ready for review on Mar 30, 2022
  41. maflcko marked this as a draft on Mar 30, 2022
  42. maflcko commented at 1:16 pm on March 31, 2022: member
    I tried bumping the CI to Windows 2022, but it failed so far: https://github.com/MarcoFalke/bitcoin-core-with-ci/compare/2203-win-%F0%9F%94%B2?expand=1
  43. DrahtBot added the label Needs rebase on Apr 5, 2022
  44. maflcko force-pushed on Apr 6, 2022
  45. DrahtBot removed the label Needs rebase on Apr 6, 2022
  46. dongcarl commented at 4:32 pm on May 13, 2022: contributor

    ACK 8857267e58a4fe24c794b3187acc67aa77a6a219

    For the libbitcoinkernel project, when decoupling from ArgsManager we introduce Options structs for many of our existing classes. Designated initializers make a lot of these code changes much clearer.

    It also seems like no one was against bumping just the msvc toolchain to C++20 in the relevant IRC chat logs: https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2022/bitcoin-core-dev.2022-03-10-19.00.log.html#l-114

    @MarcoFalke Is the CI failure related or unrelated to this change? https://cirrus-ci.com/task/4871889075044352

  47. maflcko force-pushed on May 27, 2022
  48. maflcko marked this as ready for review on May 27, 2022
  49. maflcko removed the label P2P on May 27, 2022
  50. maflcko added the label Refactoring on May 27, 2022
  51. maflcko commented at 1:05 pm on May 27, 2022: member
    Disabled for msvc due to bug for now
  52. ajtowns commented at 3:02 pm on May 27, 2022: contributor

    Disabled for msvc due to bug for now

    Ha, what a gross hack. I love it!

  53. in src/net.cpp:1118 in faa49c05dd outdated 
    1119+                Desig(fBloomFilter) node->m_bloom_filter_loaded.load(),
1120+                Desig(nKeyedNetGroup) node->nKeyedNetGroup,
1121+                Desig(prefer_evict) node->m_prefer_evict,
1122+                Desig(m_is_local) node->addr.IsLocal(),
1123+                Desig(m_network) node->ConnectedThroughNetwork(),
1124+            };
    ryanofsky commented at 6:24 pm on May 31, 2022:

    Just a thought (don’t want to bikeshed), but it might be more readable & obvious what is going on with a macro syntax like:

    0NodeEvictionCandidate candidate{
1    INIT_FIELD(id, node->GetId()),
2    INIT_FIELD(m_connected, node->m_connected),
3    ...
4};
    ajtowns commented at 7:25 pm on May 31, 2022:
    It might be easier to write a scripted-diff to convert Desig(x) to .x: than having to find the closing bracket in INIT_FIELD(id, node->GetId()), ? Might be worth a little bikeshedding to see if there’s a particularly readable format though.
    maflcko commented at 6:20 am on June 1, 2022:
    Right, I was optimizing for the scripted-diff
    laanwj commented at 12:16 pm on June 2, 2022:
    I prefer @ryanofsky ’s suggestion as well (if we need this clumsiness at all).
  54. ryanofsky approved
  55. ryanofsky commented at 6:27 pm on May 31, 2022: contributor
    Code review ACK faa49c05ddd70259d36c7eb2f9f64d120968bc95
  56. Use designated initializers fa72e0ba15
  57. in src/util/designator.h:12 in faa49c05dd outdated 
     7+
 8+/**
 9+ * Designated initializers can be used to avoid ordering mishaps in aggregate
10+ * initialization. However, they do not prevent uninitialized members.  The
11+ * checks can be disabled on msvc and other compilers by defining
12+ * DISABLE_DESIGNATED_INITIALIZER_ERRORS.
    dongcarl commented at 5:39 pm on June 1, 2022:
    Should we mention/link here that we’re blocked on the msvc upstream image?
    maflcko commented at 6:08 pm on June 1, 2022:
    Thx, expanded a bit on msvc stuff.
  58. maflcko force-pushed on Jun 1, 2022
  59. hebasto commented at 9:23 pm on June 1, 2022: member
    Concept ACK.
  60. kristapsk approved
  61. kristapsk commented at 1:53 am on June 2, 2022: contributor
    ACK fa72e0ba15c6382e9068be221ab4872bef000cbc
  62. in build_msvc/common.init.vcxproj.in:93 in fa72e0ba15 
    89@@ -90,7 +90,7 @@
90       <AdditionalOptions>/utf-8 /Zc:__cplusplus /std:c++17 %(AdditionalOptions)</AdditionalOptions>
91       <DisableSpecificWarnings>4018;4244;4267;4334;4715;4805;4834</DisableSpecificWarnings>
92       <TreatWarningAsError>true</TreatWarningAsError>
93-      <PreprocessorDefinitions>_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING;ZMQ_STATIC;NOMINMAX;WIN32;HAVE_CONFIG_H;_CRT_SECURE_NO_WARNINGS;_SCL_SECURE_NO_WARNINGS;_CONSOLE;_WIN32_WINNT=0x0601;_WIN32_IE=0x0501;WIN32_LEAN_AND_MEAN;%(PreprocessorDefinitions)</PreprocessorDefinitions>
94+      <PreprocessorDefinitions>DISABLE_DESIGNATED_INITIALIZER_ERRORS;_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING;ZMQ_STATIC;NOMINMAX;WIN32;HAVE_CONFIG_H;_CRT_SECURE_NO_WARNINGS;_SCL_SECURE_NO_WARNINGS;_CONSOLE;_WIN32_WINNT=0x0601;_WIN32_IE=0x0501;WIN32_LEAN_AND_MEAN;%(PreprocessorDefinitions)</PreprocessorDefinitions>
    hebasto commented at 9:11 am on June 2, 2022:

    This affects not only CI builds with MSVC 2019, but local builds as well which could use MSVC 2022.

    Are we going to remove it when switching to C++20?

    maflcko commented at 9:15 am on June 2, 2022:
    Yes to both questions/statements.
  63. hebasto approved
  64. hebasto commented at 9:26 am on June 2, 2022: member
    ACK fa72e0ba15c6382e9068be221ab4872bef000cbc
  65. maflcko merged this on Jun 2, 2022
  66. maflcko closed this on Jun 2, 2022
  67. in src/util/designator.h:16 in fa72e0ba15 
    11+ * checks can be disabled by defining DISABLE_DESIGNATED_INITIALIZER_ERRORS.
12+ * This should only be needed on MSVC 2019. MSVC 2022 supports them with the
13+ * option "/std:c++20"
14+ */
15+#ifndef DISABLE_DESIGNATED_INITIALIZER_ERRORS
16+#define Desig(field_name) .field_name =
    laanwj commented at 11:10 am on June 2, 2022:

    FWIW, I don’t particularly like defining our own macro for this. Would have preferred to either use the language feature as-is, or not. (inserting loose tokens like this is also a bit ugly imo, if you really want to define a macro for this why not

    0#define Desig(field_name, value) .field_name = (value)

    )

    maflcko commented at 11:15 am on June 2, 2022:
    Fair enough, no objection to revert it.
    laanwj commented at 11:38 am on June 2, 2022:
    Do we really need to support MSVC 2019?
    sipsorcery commented at 11:40 am on June 2, 2022:
    Cirrus doesn’t supprt VS2022 yet https://cirrus-ci.org/guide/windows/.
    maflcko commented at 11:41 am on June 2, 2022:

    Do we really need to support MSVC 2019?

    No, that’s the point. We should stop supporting it, as it is know to be broken (it can’t compile c++20, even though the flag exists). However, no one was successful in bumping the CI task to msvc22.

    See my previous attempt: #24531 (comment), as well as: https://github.com/cirruslabs/cirrus-ci-docs/issues/985

    Maybe a bounty task? :sweat_smile: @jamesob

    maflcko commented at 11:43 am on June 2, 2022:

    #define Desig(field_name, value) .field_name = (value)

    see previous discussion: #24531 (review)

    hebasto commented at 12:20 pm on June 25, 2022:
    #25472 :tiger2:
  68. maflcko deleted the branch on Jun 2, 2022
  69. sidhujag referenced this in commit 21d3fe60a0 on Jun 2, 2022
  70. maflcko referenced this in commit fe5911ee04 on Jun 27, 2022
  71. fanquake referenced this in commit c30b3e90f0 on Jul 13, 2022
  72. in src/util/designator.h:18 in fa72e0ba15 
    13+ * option "/std:c++20"
14+ */
15+#ifndef DISABLE_DESIGNATED_INITIALIZER_ERRORS
16+#define Desig(field_name) .field_name =
17+#else
18+#define Desig(field_name)
    maflcko commented at 8:55 am on February 14, 2023:
    Just for clarity and documentation. This code is wrong as-is and has long been removed. Omitting the field name here can lead to bugs, see https://godbolt.org/z/9TnoMKMT3 for a playground.
  73. maflcko added the label Bug on Feb 14, 2023
  74. maflcko removed the label Refactoring on Feb 14, 2023
  75. bitcoin locked this on Feb 14, 2024


maflcko jonatack luke-jr ajtowns sipsorcery hebasto DrahtBot dongcarl ryanofsky laanwj kristapsk

Labels
Bug

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-26 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me