Add 64-bit versions of core power of 2 functions #106606
Conversation
|
Related to #91664, which also touched these functions. |
There was a problem hiding this comment.
Makes total sense to have uint64_t versions.
I think this is a good opportunity to clean up surrounding mess. You cleaned up some of it already, but I'd also like to see at least constexpr being added to all functions (instead of static and inline).
While you're at it, please also remove CowData::next_po2, and call next_power_of_2 instead.
I'm also a bit uneasy about all of this rarely used boilerplate being in typedefs.h. Math namespace doesn't seem appropriate either. Perhaps we should add a new file for this, and include it where needed?
|
@Ivorforce Yeah, I agree, |
There was a problem hiding this comment.
Looks good to me. Thanks for making sure all callers are still calling what they were calling earlier.
Just one more comment for a small change.
Yeah, I agree, typedefs.h feels like the wrong place for these functions. Since, you know, they're not typedefs. But I am not proposing to change that in this PR.
That's fine. We can propose a move some other time.
| @@ -39,7 +39,7 @@ class FileAccessCompressed : public FileAccess { | |||
| bool writing = false; | |||
| uint64_t write_pos = 0; | |||
| uint8_t *write_ptr = nullptr; | |||
| uint32_t write_buffer_size = 0; | |||
| uint64_t write_buffer_size = 0; | |||
There was a problem hiding this comment.
Sneaking some changes in here already? :P
There was a problem hiding this comment.
Making FileAccessCompressed call the 64-bit version is the motivation behind the rest of the changes, and is actually enough to make the function not crash for the test in Calinou's comment (to my surprise).
We already have a templated version of |
|
I don’t suggest changing code but what was the reasoning between unsigned and signed 64 bit integers. |
|
@fire These functions exclusively work with only positive numbers due to the way they are implemented with bit shifting, so having them be unsigned is essentially self-documenting this. Negative numbers would not work. Making these unsigned also means you can go up to 2^31 and 2^63 whereas signed ints can only hold up to 2^31 - 1 and 2^63 - 1. |
There was a problem hiding this comment.
Code looks good to me. Combining this PR with #106190 makes the testing project from #106190 (review) work 🙂
This PR adds 64-bit versions of the power of 2 functions to core. Almost all uses throughout the codebase are still using the 32-bit versions of the functions, but now casts have been added to make this explicit.
A 64-bit version of
next_power_of_2is required to fix FileAccessCompressed with file sizes above 2 GiB. As for the other functions, I figured it would be consistent to change all of them at once, and this enables any other parts of the codebase to migrate to 64-bit if needed. But if it's not desired, I can remove most of the 64-bit versions of the functions.The 32-bit versions of the functions previously used
unsigned int, but this PR updates them touint32_t.I changed
nearest_shiftto use a do/while loop, so that theivariable can be unsigned, avoiding casting from signed to unsigned every loop iteration. The old>= 0check would always be true for unsigned ints.I believe this code is correct, but since these are important core functions, we should test this.