Replace memset with constexpr fill_n in bigint::align and fix memset count to account for sizeof T #4471
Conversation
…t in our call to memset with the sizeof T to clear the whole memory block.
|
What compiler/version are you referring to and what is the full error? |
There was a problem hiding this comment.
Thanks for the PR but please provide more details per my earlier comment and address warnings reported in CI: https://github.com/fmtlib/fmt/actions/runs/15737439842/job/44421626034?pr=4471
I couldn't reproduce this on public compilers but the error message recieved was
Will work on getting the builds green, thanks |
…c assert. Use a 0U to initialize bigits_.data()
| @@ -2799,7 +2800,7 @@ class bigint { | |||
| bigits_.resize(to_unsigned(num_bigits + exp_difference)); | |||
| for (int i = num_bigits - 1, j = i + exp_difference; i >= 0; --i, --j) | |||
| bigits_[j] = bigits_[i]; | |||
| memset(bigits_.data(), 0, to_unsigned(exp_difference) * sizeof(bigit)); | |||
| fill_n(bigits_.data(), to_unsigned(exp_difference), 0U); | |||
There was a problem hiding this comment.
bigit is a uint32_t so bigits_.data() returns a uint32_t*. 0U is the unsigned literal for 0. I could use fill_n(bigits_.data(), to_unsigned(exp_difference), to_unsigned(0)); if that's better but thought using the literal U would convey the same meaning. LMK if you prefer I change it to something else. The 0 is by default signed so I got a signed/unsigned warning.
…ross multiple lines
|
Merged, thanks! |
Had an issue with a particularly pedantic compiler that flagged the use of memset (a non-constexpr labeled function) in the use of bigint::align when it's marked FMT_CONSTEXPR=constexpr.
Using the internal fill_n allows the function to stay constexpr and use memset when it isn't.
Also noticed that when memset is used in fill_n, it doesn't account for sizeof(T) so when using fill_n to clear a large buffer, memset will leave a portion of the buffer unmodified.