refactor: remove windows-only compat.h usage in random #26814

pull fanquake wants to merge 3 commits into bitcoin:master from fanquake:prune_compat_random changing 1 files +9 −10

fanquake commented at 6:02 PM on January 4, 2023: member

This change is related to removing the use of compat.h as a miscellaneous catch-all for unclear/platform specific includes. Somewhat prompted by IWYU-related discussion here: https://github.com/bitcoin/bitcoin/pull/26763/files#r1058861693.

The only reason compat.h is required in random.cpp for Windows (note the #ifdef WIN32), is for ssize_t and an "indirect" inclusion of windows.h. I say indirect, because windows.h isn't actually included in compat.h either, it's dragged in as a side-effect of other windows includes there, i.e winsock2.h.

Remove this coupling by replacing ssize_t with int, just including windows.h and removing compat.h.
DrahtBot commented at 6:02 PM on January 4, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK hebasto, john-moffett

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
maflcko added the label DrahtBot Guix build requested on Jan 5, 2023
maflcko renamed this:
~~random: remove windows-only compat.h usage~~
refactor: remove windows-only compat.h usage in random
on Jan 5, 2023
hebasto commented at 11:29 AM on January 5, 2023: member

Concept ACK.

Remove this coupling by replacing ssize_t with int, just including windows.h and removing compat.h.

Why windows.h is required in here?
fanquake commented at 11:31 AM on January 5, 2023: member

Why windows.h is required in here?

For the windows cryptography functions & types. You can't use wincrypt.h without windows.h.
hebasto approved
hebasto commented at 12:33 PM on January 5, 2023: member

ACK 4e7bed297a6dd1ae0da52a7eb99659849f18f259

~As windows.h includes wincrypt.h, we can provide the NOCRYPT macro, along with WIN32_LEAN_AND_MEAN and NOMINMAX ones, to make other translation units, which #include <windows.h>, smaller.~

nm, WIN32_LEAN_AND_MEAN implies NOCRYPT.

Type	Reviewers
ACK	hebasto, john-moffett

DrahtBot commented at 6:57 PM on January 5, 2023: contributor

Guix builds

File	commit 296e88225096125b08665b97715c5b8ebb1d28ec<br>(master)	commit b8180efb9080ce5d5e99a46aab79e6813fa1a592<br>(master and this pull)
SHA256SUMS.part	`647bc157e5a10955...`	`10b8b3c292fc3f1d...`
*-aarch64-linux-gnu-debug.tar.gz	`6513dd05a548ddb0...`	`4aec405ebc584327...`
*-aarch64-linux-gnu.tar.gz	`b0a473b42e7ecbd2...`	`ad5f20be126da0c6...`
*-arm-linux-gnueabihf-debug.tar.gz	`975af99afc1cc105...`	`d75442d05a506d99...`
*-arm-linux-gnueabihf.tar.gz	`a17043b160936d03...`	`a1a5a91bbd058679...`
*-arm64-apple-darwin-unsigned.dmg	`91331a250e17c97a...`	`b1bd894f9839cb37...`
*-arm64-apple-darwin-unsigned.tar.gz	`4c5ebca4b051cd31...`	`35f57e91f00fce81...`
*-arm64-apple-darwin.tar.gz	`e6dce1673d3a725e...`	`faf726d3c8060ba3...`
*-powerpc64-linux-gnu-debug.tar.gz	`ce3b136e0253a24a...`	`e21ea7a6ad1e6699...`
*-powerpc64-linux-gnu.tar.gz	`a0bce809a414ee83...`	`bbe00e2340d5bf26...`
*-powerpc64le-linux-gnu-debug.tar.gz	`4de4ca017f52d204...`	`b2ffbb99c8c4508f...`
*-powerpc64le-linux-gnu.tar.gz	`12a9a9f473af73a8...`	`11e0715854037a54...`
*-riscv64-linux-gnu-debug.tar.gz	`462463139585f09e...`	`da6bf1f55a5d8a60...`
*-riscv64-linux-gnu.tar.gz	`5123822cc71e27c7...`	`20049a39cd607bfd...`
*-win64-debug.zip	`07d8223bbfc29df4...`	`41cbcb02df0796ac...`
*-win64-setup-unsigned.exe	`6687c9a5d5d3c027...`	`999341abf8bd35a7...`
*-win64-unsigned.tar.gz	`1cf4b17c087f3021...`	`dec0d3b7d28a5050...`
*-win64.zip	`176917cdfa005dba...`	`e5e6d4a351576c3b...`
*-x86_64-apple-darwin-unsigned.dmg	`b7e17866f1ec3e5e...`	`68bf9da574d9eb88...`
*-x86_64-apple-darwin-unsigned.tar.gz	`726f79edd133eab0...`	`5e04bffbca91449d...`
*-x86_64-apple-darwin.tar.gz	`3d66873be5c3257f...`	`5d248aa40ceb726f...`
*-x86_64-linux-gnu-debug.tar.gz	`767b26a31fa0afeb...`	`5bbd22075cef9dbb...`
*-x86_64-linux-gnu.tar.gz	`cced91a56db28f0f...`	`d3f999f21879c9a5...`
*.tar.gz	`0c83b180e8e552c9...`	`4e7f2a05e38561d5...`
guix_build.log	`03945abec112dacd...`	`087a6b03e05b053b...`
guix_build.log.diff		`e02b41658d09ecc9...`

DrahtBot removed the label DrahtBot Guix build requested on Jan 5, 2023
DrahtBot added the label Refactoring on Jan 5, 2023
fanquake cross-referenced this on Jan 5, 2023 from issue refactor: remove windows-only compat.h usage in randomenv by fanquake
maflcko referenced this in commit 5271c77f83 on Jan 23, 2023
sidhujag referenced this in commit 18c920073b on Jan 23, 2023

random: use int for MAX_TRIES

Removing the use of ssize_t, removes the need to include compat.h, just 
to make Windows happy.

4dc12816ac

random: remove compat.h include

We no-longer need ssize_t.

Add windows.h, which was being indirectly included via compat.h. It isn't
actually included in compat.h itself, but was being included as a side-effect
of other includes, like winsock2.h.

75ec6275e6

random: consolidate WIN32 #ifdefs
Order includes Remove // for xyz comments
621cfb7722
fanquake force-pushed on Feb 17, 2023
fanquake requested review from john-moffett on Feb 17, 2023
fanquake requested review from hebasto on Feb 17, 2023
hebasto approved
hebasto commented at 4:30 PM on February 17, 2023: member
re-ACK 621cfb77227b5a240d66547947f73130f0c51f44, rebased only since my recent review. Verified with:
```
git range-diff master 4e7bed297a6dd1ae0da52a7eb99659849f18f259 621cfb77227b5a240d66547947f73130f0c51f44
```
john-moffett approved
john-moffett commented at 6:13 PM on February 17, 2023: contributor

ACK 621cfb77227b5a240d66547947f73130f0c51f44
fanquake merged this on Feb 19, 2023
fanquake closed this on Feb 19, 2023
fanquake deleted the branch on Feb 19, 2023
sidhujag referenced this in commit 318377fe1a on Feb 25, 2023
bitcoin locked this on Feb 19, 2024