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

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.

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

Guix builds

File	commit 296e88225096125b08665b97715c5b8ebb1d28ec(master)	commit b8180efb9080ce5d5e99a46aab79e6813fa1a592(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

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:

0git 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

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

Reviews

Guix builds