Use designated initializers #24531

maflcko · 2022-03-11T13:13:15Z

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

maflcko · 2022-03-11T13:13:51Z

Looks like there is a msvc bug that causes fatal error C1060: compiler is out of heap space 🤷‍♂️

jonatack · 2022-03-11T13:55:47Z

C:\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]
C:\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]
C:\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]

src/fs.h

src/scheduler.cpp

src/fs.h

maflcko · 2022-03-17T16:55:50Z

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

sipsorcery · 2022-03-17T19:44:41Z

@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?

hebasto · 2022-03-17T19:52:46Z

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

On Cirrus we have Visual Studio 2019:

> msbuild -version 
Microsoft (R) Build Engine version 16.11.2+f32259642 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.
16.11.2.50704

maflcko · 2022-03-17T20:00:13Z

Maybe we should bump it? Not sure how, though.

sipsorcery · 2022-03-18T08:58:46Z

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.

maflcko · 2022-03-18T09:01:55Z

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

hebasto · 2022-03-18T10:24:18Z

CI is green now 😄

maflcko · 2022-03-18T10:40:21Z

Taken out of draft for review.

DrahtBot · 2022-03-19T17:46:44Z

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

Conflicts

No conflicts as of last run.

maflcko · 2022-03-25T06:33:06Z

Looks like another msvc bug??

...
 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__ 
 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] 
 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 
 test  2022-03-24T22:07:43.264000Z TestFramework (ERROR): Unexpected exception caught during testing 
                                   Traceback (most recent call last):
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
                                       self.run_test()
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\p2p_getaddr_caching.py", line 52, in run_test
                                       self.nodes[0].addpeeraddress(a, 8333)
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\coverage.py", line 49, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 142, in __call__
                                       response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 107, in _request
                                       self.__conn.request(method, path, postdata, headers)
                                     File "C:\Python39\lib\http\client.py", line 1257, in request
                                       self._send_request(method, url, body, headers, encode_chunked)
                                     File "C:\Python39\lib\http\client.py", line 1303, in _send_request
                                       self.endheaders(body, encode_chunked=encode_chunked)
                                     File "C:\Python39\lib\http\client.py", line 1252, in endheaders
                                       self._send_output(message_body, encode_chunked=encode_chunked)
                                     File "C:\Python39\lib\http\client.py", line 1012, in _send_output
                                       self.send(msg)
                                     File "C:\Python39\lib\http\client.py", line 952, in send
                                       self.connect()
                                     File "C:\Python39\lib\http\client.py", line 925, in connect
                                       self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
                                   OSError: [WinError 10022] An invalid argument was supplied
 test  2022-03-24T22:07:43.264000Z TestFramework (DEBUG): Closing down network thread 
 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 
 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__ 
 test  2022-03-24T22:07:43.327000Z TestFramework (INFO): Stopping nodes 
 test  2022-03-24T22:07:43.327000Z TestFramework.node0 (DEBUG): Stopping node 
 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 
 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__ 
...

maflcko · 2022-05-27T13:05:37Z

Disabled for msvc due to bug for now

ajtowns · 2022-05-27T15:02:17Z

Disabled for msvc due to bug for now

Ha, what a gross hack. I love it!

ryanofsky

Code review ACK faa49c0

ryanofsky · 2022-05-31T18:24:31Z

src/net.cpp

+                Desig(prefer_evict) node->m_prefer_evict,
+                Desig(m_is_local) node->addr.IsLocal(),
+                Desig(m_network) node->ConnectedThroughNetwork(),
+            };


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

NodeEvictionCandidate candidate{ INIT_FIELD(id, node->GetId()), INIT_FIELD(m_connected, node->m_connected), ... };

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.

Right, I was optimizing for the scripted-diff

I prefer @ryanofsky 's suggestion as well (if we need this clumsiness at all).

src/util/designator.h

hebasto · 2022-06-01T21:23:53Z

Concept ACK.

kristapsk

ACK fa72e0b

hebasto · 2022-06-02T09:11:46Z

build_msvc/common.init.vcxproj.in

@@ -90,7 +90,7 @@
      <AdditionalOptions>/utf-8 /Zc:__cplusplus /std:c++17 %(AdditionalOptions)</AdditionalOptions>
      <DisableSpecificWarnings>4018;4244;4267;4334;4715;4805;4834</DisableSpecificWarnings>
      <TreatWarningAsError>true</TreatWarningAsError>
-      <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>
+      <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>


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?

Yes to both questions/statements.

hebasto

ACK fa72e0b

laanwj · 2022-06-02T11:10:58Z

src/util/designator.h

+ * option "/std:c++20"
+ */
+#ifndef DISABLE_DESIGNATED_INITIALIZER_ERRORS
+#define Desig(field_name) .field_name =


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

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

)

Fair enough, no objection to revert it.

Do we really need to support MSVC 2019?

Cirrus doesn't supprt VS2022 yet https://cirrus-ci.org/guide/windows/.

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: cirruslabs/cirrus-ci-docs#985

Maybe a bounty task? 😅 @jamesob

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

see previous discussion: #24531 (comment)

#25472 🐅

…ualstudio2022` 05b2d9f build: Bump default `PlatformToolset` for Visual Studio 2022 (Hennadii Stepanov) 460c6c7 doc: Make Windows build docs match the CI task (Hennadii Stepanov) 849cf96 ci: Increase CPU number for "Win64 native" task (Hennadii Stepanov) a18c4c1 ci: Bump vcpkg to the latest version (Hennadii Stepanov) b9a5a9b ci: Limit ccache cache size properly on "Win64 native" task (Hennadii Stepanov) 156bc89 ci: Update Windows task image up to visualstudio2022 (Hennadii Stepanov) Pull request description: Besides upgrading Visual Studio, which seems [inevitable](bitcoin/bitcoin#24531 (comment)), this PR also: - bumps vcpkg to the latest version (previous one was in bitcoin/bitcoin#24847) - fixes cache size limit for `ccache` ACKs for top commit: sipsorcery: reACK 05b2d9f. hebasto: > ACK [05b2d9f](bitcoin/bitcoin@05b2d9f) jarolrod: ACK 05b2d9f Tree-SHA512: 6338e74a3f1907f09ca29540e9e2cf7ac3be3b9e28271e8a20e71b67a9c3d5ebb8d34528b9677bcd1d9bc0ad723d68fd2ba7db368443ed1854cca3a3961f294b

630c171 refactor: Drop no longer needed `util/designator.h` (Hennadii Stepanov) 88ec5d4 build: Increase MS Visual Studio minimum version (Hennadii Stepanov) 555f9dd rpc, refactor: Add `decodepsbt_outputs` (Hennadii Stepanov) 0c432cb rpc, refactor: Add `decodepsbt_inputs` (Hennadii Stepanov) 01d95a3 rpc, refactor: Add `getblock_prevout` (Hennadii Stepanov) Pull request description: Visual Studio 2022 with `/std:c++20` supports [designated initializers](#24531). ACKs for top commit: sipsorcery: reACK 630c171. Tree-SHA512: 5b8933940dd69061c6b077512168bebb6fea05d429b63ffbab191950798b4c825e8484b1a651db0ae13f97eae481097d3c16395659c0f3b9f847af2aaf44b65d

maflcko · 2023-02-14T08:55:07Z

src/util/designator.h

+#ifndef DISABLE_DESIGNATED_INITIALIZER_ERRORS
+#define Desig(field_name) .field_name =
+#else
+#define Desig(field_name)


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.

DrahtBot added the P2P label Mar 11, 2022

luke-jr reviewed Mar 17, 2022

View reviewed changes

src/fs.h Outdated Show resolved Hide resolved

luke-jr reviewed Mar 17, 2022

View reviewed changes

src/scheduler.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2203-designated_init- branch 2 times, most recently from ee4a55e to 7106a59 Compare March 17, 2022 07:24

maflcko mentioned this pull request Mar 17, 2022

build: Add --enable-c++20 option #24169

Merged

ajtowns reviewed Mar 17, 2022

View reviewed changes

src/fs.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2203-designated_init- branch 2 times, most recently from fa2cea2 to 10b4fc9 Compare March 17, 2022 16:21

maflcko marked this pull request as ready for review March 18, 2022 09:00

maflcko force-pushed the 2203-designated_init- branch 2 times, most recently from fa14cfb to fadc9cf Compare March 18, 2022 09:03

DrahtBot mentioned this pull request Mar 20, 2022

net/net processing: Move tx inventory into net_processing #21160

Merged

maflcko marked this pull request as draft March 22, 2022 15:40

maflcko force-pushed the 2203-designated_init- branch from fadc9cf to fa42f0b Compare March 24, 2022 10:40

maflcko marked this pull request as ready for review March 24, 2022 12:22

maflcko force-pushed the 2203-designated_init- branch from fa42f0b to fad0b52 Compare March 24, 2022 13:42

maflcko mentioned this pull request Mar 25, 2022

doc: document clang tidy named args #24665

Merged

dongcarl mentioned this pull request May 17, 2022

[kernel 2b/n] Add ChainstateManager::m_adjusted_time_callback #25064

Merged

maflcko force-pushed the 2203-designated_init- branch from 8857267 to faa49c0 Compare May 27, 2022 09:45

maflcko marked this pull request as ready for review May 27, 2022 10:52

maflcko added Refactoring and removed P2P labels May 27, 2022

ryanofsky approved these changes May 31, 2022

View reviewed changes

dongcarl reviewed Jun 1, 2022

View reviewed changes

src/util/designator.h Outdated Show resolved Hide resolved

Use designated initializers

fa72e0b

maflcko force-pushed the 2203-designated_init- branch from faa49c0 to fa72e0b Compare June 1, 2022 18:06

kristapsk approved these changes Jun 2, 2022

View reviewed changes

hebasto reviewed Jun 2, 2022

View reviewed changes

hebasto approved these changes Jun 2, 2022

View reviewed changes

maflcko merged commit 39ddd52 into bitcoin:master Jun 2, 2022

laanwj reviewed Jun 2, 2022

View reviewed changes

maflcko deleted the 2203-designated_init-🛍 branch June 2, 2022 11:12

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 2, 2022

Merge bitcoin#24531: Use designated initializers

21d3fe6

This was referenced Jun 25, 2022

build: Increase MS Visual Studio minimum version #25472

Merged

ci: Update Windows task image up to visualstudio2022 #25460

Merged

ryanofsky mentioned this pull request Aug 24, 2022

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

hebasto mentioned this pull request Feb 13, 2023

build: Add CMake-based build system (1 of N) #27060

Closed

maflcko commented Feb 14, 2023

View reviewed changes

maflcko added Bug and removed Refactoring labels Feb 14, 2023

hebasto mentioned this pull request Aug 7, 2023

error C3203: 'UniqueLock' #28229

Closed

1 task

bitcoin locked and limited conversation to collaborators Feb 14, 2024

Use designated initializers #24531

Use designated initializers #24531

Uh oh!

Conversation

maflcko commented Mar 11, 2022

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

jonatack commented Mar 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Mar 17, 2022

Uh oh!

sipsorcery commented Mar 17, 2022

Uh oh!

hebasto commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 17, 2022

Uh oh!

sipsorcery commented Mar 18, 2022

Uh oh!

maflcko commented Mar 18, 2022

Uh oh!

hebasto commented Mar 18, 2022

Uh oh!

maflcko commented Mar 18, 2022

Uh oh!

DrahtBot commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Mar 25, 2022

Uh oh!

maflcko commented May 27, 2022

Uh oh!

ajtowns commented May 27, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jun 1, 2022

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 17, 2022 •

edited

Loading

DrahtBot commented Mar 19, 2022 •

edited

Loading

laanwj Jun 2, 2022 •

edited

Loading

laanwj Jun 2, 2022 •

edited

Loading

maflcko Jun 2, 2022 •

edited

Loading