doc: remove // for ... comments

fanquake commented at 11:41 am on May 19, 2025: member

We don’t add or maintain these, and they are of little value, as well as having the effect of polluting diffs, if changed.

They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in validation.h.

DrahtBot commented at 11:41 am on May 19, 2025: contributor

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/32562.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, willcl-ark, fjahr
Concept ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/870 ([DRAFT] Expose AssumeUTXO Load Snapshot Functionality To The GUI by D33r-Gee)
#bitcoin-core/gui/807 (refactor: interfaces, make ‘createTransaction’ less error-prone by furszy)
#32380 (Modernize use of UTF-8 in Windows code by hebasto)
#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
#31981 (Add checkBlock() to Mining interface by Sjors)
#31864 (doc: add missing copyright headers by fanquake)
#31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Docs on May 19, 2025

in src/crypto/sha3.cpp:14 in 3cf3695910 outdated

 9@@ -10,10 +10,10 @@
10 #include <span.h>
11 
12 #include <algorithm>
13-#include <array> // For std::begin and std::end.
14+#include <array>
15 #include <bit>
16-
17-#include <stdint.h>
18+#include <cstdint>

Sjors commented at 12:16 pm on May 19, 2025:

3cf3695910f332f1aa8af57de7a01f63d5d7dc68: maybe change this in a separate commit?

Similar in other files, or is this auto generated?

Sjors commented at 12:19 pm on May 19, 2025: member

Concept ACK

Do we have a linter for unused includes?

No opinion on the seemingly unrelated WIN32 changes.

fanquake commented at 12:21 pm on May 19, 2025: member

Do we have a linter for unused includes?

No includes are being removed here, so I don’t see how that’s relevant? IWYU would be the closest tool you could run.

willcl-ark commented at 12:44 pm on May 19, 2025: member

Concept ACK.

https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/validationinterface.h#L11 https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/cuckoocache.h#L10

The only others I found were in subtrees.

init: cerrno is used on all platforms 19ba499b1f

net: drop win32 ifdef 1b9cdc933f

fanquake force-pushed on May 19, 2025

fanquake commented at 12:46 pm on May 19, 2025: member

Thanks, dropped.

fjahr commented at 12:49 pm on May 19, 2025: contributor

Concept ACK

I kind of like the idea but seems way too much effort to actually maintain this in a consistent way for very little gain. I don’t remember if any of these ever saved me time, probably not because my workflow doesn’t rely on them and most people probably use an editor that allows them to jump to definition.

Sjors commented at 1:00 pm on May 19, 2025: member

Do we have a linter for unused includes?

No includes are being removed here, so I don’t see how that’s relevant? IWYU would be the closest tool you could run.

The comments were useful for knowing when an include could be dropped (though not if they’re incomplete).

willcl-ark approved

willcl-ark commented at 1:24 pm on May 19, 2025: member

ACK e4aee0b3a23a9609e16bd7c8eda618dcf0a8b1db

DrahtBot requested review from fjahr on May 19, 2025

DrahtBot requested review from Sjors on May 19, 2025

DrahtBot added the label CI failed on May 19, 2025

in src/support/lockedpool.cpp:13 in e4aee0b3a2 outdated

12-#include <sys/resource.h> // for getrlimit
13-#include <limits.h> // for PAGESIZE
14-#include <unistd.h> // for sysconf
15+#include <sys/mman.h>
16+#include <sys/resource.h>
17+#include <limits.h>

stickies-v commented at 2:55 pm on May 19, 2025:

nit: #include <limits.h> can be removed according to IWYU I suspect because we also #include <limits>

in src/crypto/sha3.cpp:15 in e4aee0b3a2 outdated

14+#include <array>
15 #include <bit>
16-
17-#include <stdint.h>
18+#include <cstdint>
19+#include <span>

stickies-v commented at 2:58 pm on May 19, 2025:

nit: the span.h include can now be removed according to IWYU

in src/test/util_tests.cpp:30 in e4aee0b3a2 outdated

25@@ -26,12 +26,13 @@
26 
27 #include <array>
28 #include <cmath>
29+#include <cstdint>
30+#include <cstring>

stickies-v commented at 3:01 pm on May 19, 2025:

nit: cstring includes can just be dropped according to IWYU

stickies-v approved

stickies-v commented at 3:05 pm on May 19, 2025: contributor

ACK e4aee0b3a23a9609e16bd7c8eda618dcf0a8b1db

The new includes seem arbitrary, they usually still don’t represent the full include list (according to IWYU), and could be dropped, but I verified them against the IWYU run and mostly seem fine (I added comments for the ones that seem incorrect.

Found a couple more include comments in util/fs_helpers.cpp

doc: remove For ... comments

We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.

They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.

7193245cd6

fanquake force-pushed on May 19, 2025

stickies-v commented at 3:47 pm on May 19, 2025: contributor

re-ACK 7193245cd66791216d4e586ca09302b26d4b7528

DrahtBot requested review from willcl-ark on May 19, 2025

willcl-ark approved

willcl-ark commented at 5:10 pm on May 19, 2025: member

reACK 7193245cd66791216d4e586ca09302b26d4b7528

DrahtBot removed the label CI failed on May 19, 2025

fjahr commented at 8:21 pm on May 19, 2025: contributor

ACK 7193245cd66791216d4e586ca09302b26d4b7528

Confirmed there are no such comments left outside of subtrees. Also seeing different full include lists suggested by IWYU but those seem exaggerated for this change here. Checked a hand full of the files that had bigger changes and didn’t find anything wrong.

fanquake merged this on May 20, 2025

fanquake closed this on May 20, 2025

fanquake deleted the branch on May 20, 2025

doc: remove // for … comments #32562

Code Coverage & Benchmarks

Reviews

Conflicts