subprocess: Fix -Wunused-private-field when building with clang-cl on Windows

[hebasto]

This PR is a prerequisite for #31507.

It resolves -Wunused-private-field warnings triggered in src/util/subprocess.h when compiling with clang-cl on Windows:

D:\a\bitcoin\bitcoin\src\util/subprocess.h(759,10): warning : private field 'parent_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(760,7): warning : private field 'err_wr_pipe_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(1038,7): warning : private field 'child_pid_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]

Only the second commit has been submitted upstream. The first commit is specific to this repository and not applicable upstream.

[DrahtBot]

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34385.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, purpleKarrot, sedited

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

[fanquake]

Context and details can be found in #31507 (comment).

Can you put the relevent context and details in this PR, and in the commit messages, rather than linking to a comment in a different PR.

[fanquake]

Can these be fixed with a few [[maybe_unused]] rather than a bunch of #ifdefs and shuffling code around?

[maflcko]

i think it is fine to not compile fork-exec-child on Windows, if it doesn't exist there conceptually.

[hebasto]

The point was to make these changes compatible with the upstream, which uses C++11 standard.

[fanquake]

Sure, but it seems a bit odd at this point, to try and remain compatible with upstream, which uses an older standard, is getting little-to-no maintainence, seems to be generally low code quality, and we are continuing to make header changes that diverge.

[maflcko]

I still have a slight preference to guard this behind __USING_WINDOWS__. It seems cleaner, but no strong opinion.

[hebasto]

Context and details can be found in #31507 (comment).

Can you put the relevent context and details in this PR, and in the commit messages, rather than linking to a comment in a different PR.

Thanks! Rewritten.

[maflcko]

review ACK 1b36bf0 👋

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 1b36bf0c5d71ea2e7e3595eb10611ea1a8c3a0d1 👋
I9U6zcesjW0zlruquf+ALGeAuSFK660ouAJH0/n7Xkf7OQwvGsygHSjtVea/fxa4i0lFllOOqsuco5UOq8kiBQ==

[hebasto]

... the second commit has been submitted upstream.

It is landed now.

[purpleKarrot]

ACK 1b36bf0

Note that the C++ standard reserves names that contain two consecutive underscores, so strictly speaking, this is not valid C++. Unrelated, I know.

[maflcko]

Note that the C++ standard reserves names that contain two consecutive underscores, so strictly speaking, this is not valid C++. Unrelated, I know.

Maybe a scripted-diff upstream and here could fix this 😅

[purpleKarrot]

Another thing that comes to mind is that it is weird to conclude the target platform from he compiler identifier:

#if (defined _MSC_VER) || (defined __MINGW32__)
  #define __USING_WINDOWS__
#endif

This code is assuming, that if either MSVC or MINGW are used as the compiler, the target platform must be windows. But why not use _WIN32 directly whenever the win32 API is targeted? It is already set by all compilers.

[fanquake]

Yea. Rather than introducing more redundant, technically invalid C++, to stay compatible with upstream, we could just diverge, and write better code.

[sedited]

ACK 1b36bf0

At this point we are diverged anyway, so I'd suggest renaming __USING_WINDOWS__ in a follow-up.