refactor: use _MiB/_GiB consistently for byte conversions#34435
refactor: use _MiB/_GiB consistently for byte conversions#34435l0rinc wants to merge 2 commits intobitcoin:masterfrom
_MiB/_GiB consistently for byte conversions#34435Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
If you want, I can propose adding explanatory named comments (e.g. /cache_bytes=/) for these third-or-later literal arguments to improve readability. 2026-03-06 21:45:29 |
b616af9 to
db3ada7
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
db3ada7 to
05d23f6
Compare
05d23f6 to
0768e6b
Compare
| static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; } | ||
| static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; } |
There was a problem hiding this comment.
I wonder if multiplying with size_t instead of unspecified integer expression risks flipping negative function inputs into positive output values, but it's probably only used with positive inputs regardless.
There was a problem hiding this comment.
Line 35 in 25884bd
There was a problem hiding this comment.
Experimented a bit and it seems behavior hasn't changed. Passing test:
constexpr uint64_t GB_BYTES{1000000000};
static inline int PruneMiBtoGB_old(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; }
static inline int PruneMiBtoGB_new(int64_t mib) { return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; }
static inline int64_t PruneGBtoMiB_old(int gb) { return gb * GB_BYTES / 1024 / 1024; }
static inline int64_t PruneGBtoMiB_new(int gb) { return gb * GB_BYTES / 1_MiB; }
BOOST_AUTO_TEST_CASE(prune_tests)
{
BOOST_CHECK_EQUAL(PruneMiBtoGB_old(1500), 2);
BOOST_CHECK_EQUAL(PruneMiBtoGB_new(1500), 2);
BOOST_CHECK_EQUAL(PruneMiBtoGB_old(-1500), 1266874889);
BOOST_CHECK_EQUAL(PruneMiBtoGB_new(-1500), 1266874889);
BOOST_CHECK_EQUAL(PruneGBtoMiB_old(1), 953);
BOOST_CHECK_EQUAL(PruneGBtoMiB_new(1), 953);
BOOST_CHECK_EQUAL(PruneGBtoMiB_old(-1), 17592186043462);
BOOST_CHECK_EQUAL(PruneGBtoMiB_new(-1), 17592186043462);
}dfe9038 to
e5b573f
Compare
hodlinator
left a comment
There was a problem hiding this comment.
Concept ACK e5b573f
Makes the code more readible.
Examples:
-constexpr unsigned int FLTR_FILE_CHUNK_SIZE = 0x100000; // 1 MiB
+constexpr unsigned int FLTR_FILE_CHUNK_SIZE = 1_MiB;
-if (m_total_allocated > 0x1000000) return;
+if (m_total_allocated > 16_MiB) return;
-cuckoo_cache.setup_bytes(megabytes << 20);
+cuckoo_cache.setup_bytes(megabytes * 1_MiB);
-LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0));
+LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() / double(1_MiB));
-constexpr uint64_t min_disk_space = 52428800; // 50 MiB
+constexpr uint64_t min_disk_space = 50_MiB;| static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; } | ||
| static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; } |
There was a problem hiding this comment.
Experimented a bit and it seems behavior hasn't changed. Passing test:
constexpr uint64_t GB_BYTES{1000000000};
static inline int PruneMiBtoGB_old(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; }
static inline int PruneMiBtoGB_new(int64_t mib) { return (mib * 1_MiB + GB_BYTES - 1) / GB_BYTES; }
static inline int64_t PruneGBtoMiB_old(int gb) { return gb * GB_BYTES / 1024 / 1024; }
static inline int64_t PruneGBtoMiB_new(int gb) { return gb * GB_BYTES / 1_MiB; }
BOOST_AUTO_TEST_CASE(prune_tests)
{
BOOST_CHECK_EQUAL(PruneMiBtoGB_old(1500), 2);
BOOST_CHECK_EQUAL(PruneMiBtoGB_new(1500), 2);
BOOST_CHECK_EQUAL(PruneMiBtoGB_old(-1500), 1266874889);
BOOST_CHECK_EQUAL(PruneMiBtoGB_new(-1500), 1266874889);
BOOST_CHECK_EQUAL(PruneGBtoMiB_old(1), 953);
BOOST_CHECK_EQUAL(PruneGBtoMiB_new(1), 953);
BOOST_CHECK_EQUAL(PruneGBtoMiB_old(-1), 17592186043462);
BOOST_CHECK_EQUAL(PruneGBtoMiB_new(-1), 17592186043462);
}e5b573f to
792d679
Compare
There was a problem hiding this comment.
ACK 792d679
Concpt. review: #34435 (review)
Checked trickier hex literals actually match _MiB/_GiB-versions.
Diffed against previously reviewed push and aside from commit re-ordering, only saw minimal deduplication in gib_string_literal_test.
6091d91 to
2c35b07
Compare
6b5faad to
6069bc9
Compare
hodlinator
left a comment
There was a problem hiding this comment.
re-ACK 6069bc9
Just changes to surrounding diff context and WARN_FLUSH_COINS_SIZE being replaced by constant no longer measured in bytes.
328ddc4 to
c8e955c
Compare
hodlinator
left a comment
There was a problem hiding this comment.
re-ACK c8e955c
(Was just rebased to kick CI).
| constexpr auto max_gib{std::numeric_limits<size_t>::max() >> 30}; | ||
| if constexpr (SIZE_MAX == UINT32_MAX) { | ||
| BOOST_CHECK_EQUAL(max_gib, 3U); | ||
| BOOST_CHECK_EXCEPTION(4_GiB, std::overflow_error, HasReason("GiB value too large for size_t byte conversion")); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the suggestion, 32-bit architectures should also support this widening-first approach, so I added it to the test suite.
I dislike size_t too (for the exact reasons you mentioned), and I’d gladly review a PR that addresses that, but it seems more involved than this one.
There was a problem hiding this comment.
About size_t being annoying, see also #34692 (comment) 😅
c8e955c to
02184de
Compare
Introduce `operator""_GiB`, sharing the overflow-checked conversion logic with the existing `operator""_MiB`. Use `1_GiB` in a few existing places where it is a drop-in replacement (e.g. `1024_MiB`, `1<<30`) and extend unit tests to cover boundary behavior.
Replace hard-coded MiB byte conversions (e.g. `1024*1024`, `1<<20`, `1048576`) with the existing `_MiB` literal to improve readability and avoid repeating constants. In the few spots where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never turn negative. Extend unit tests to cover the 32-bit `size_t` overflow boundary and to assert equivalence for integer and floating-point conversions.
02184de to
85b9d0b
Compare
sedited
left a comment
There was a problem hiding this comment.
Concept ACK
I'm not sure about the first commit, seems a bit noisy to do a conversion from 1024 MiB to 1 GiB for a developer who is probably aware of that fact since shortly after using a computer for the first time. For me at least it is also a good reminder that the unit of -dbcache is MiBs.
The second commit seems like a decent readability improvement though.
| BOOST_CHECK_EQUAL(50_MiB / 1_MiB, 50U); | ||
| BOOST_CHECK_EQUAL(50_MiB, 52428800U); | ||
| BOOST_CHECK_EQUAL(128_MiB, 0x8000000U); | ||
| BOOST_CHECK_EQUAL(550_MiB, 550ULL * 1024 * 1024); |
There was a problem hiding this comment.
Are these really useful? Feels a bit like testing BOOST_CHECK_EQUAL(1+1,2).
There was a problem hiding this comment.
The reviewers can decide. I covered every change category that I modified in this commit as proof that it doesn't change behavior. I often err on the side of over-testing.
Problem
Byte-size conversions in the codebase currently show up in many equivalent formats (multiplication/division chains, shifts, hex/binary literals), which creates a maintenance burden and makes review error-prone - especially considering the architectural differences of
size_t.Inspired by #34305 (comment), it seemed appropriate to unify
Mebibyteusage across the codebase and addGibibytesupport with 32/64 bitsize_tvalidation.Fix
This PR refactors those call sites to use
""_MiB(existing) and""_GiB(new), and adds the encountered value/pattern replacements to unit tests to make review straightforward, and to ensure the conversions remain valid.The literals are overflow-checked when converting to
size_t, and unit tests cover the 32-bit boundary cases.Concretely, it replaces patterns such as:
1024*1024,1<<20,0x100000,1048576,/ 1024 / 1024,* (1.0 / 1024 / 1024)→1_MiBordouble(1_MiB)1024*1024*1024,1<<30,0x40000000,1024_MiB,>> 30→1_GiBNote
In the few places where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never become negative.