-
Type:
Improvement
-
Resolution: Fixed
-
Priority:
Major - P3
-
Affects Version/s: 4.3 Desired
-
Component/s: None
-
None
-
Fully Compatible
-
Dev Tools 2019-08-26, Dev Tools 2019-09-09
-
None
-
None
-
None
-
None
-
None
-
None
-
None
This header provides overflow-protected versions of multiply, add, and subtract over int64_t and uint64_t types. There are a few problems though.
1) The overload set provided for MSVC is different than the one for GCC/Clang.
The functions are declared at the top of the file and implemented below, but the clang provides overloads for the long and long long ranks instead of the uint64_t alias used in the declarations. So essentially the declarations are not representative of what's provided in the definitions part of the file and we can just get rid of it.
2) The names are redundant:
E.g.: mongoUnsignedMultiplyOverflow64(...)
The "mongo" is already the namespace in which the function appears.
The "Unsigned" is easily deduced from the type of the output argument.
The "64" as well, and we provide a "long" rank which might not be 64 bit.
All that's needed is something to mean "Multiply" and something to mean "Overflow".
3) Twisty little overloads all different.... The noise in the names makes it hard to tell what's going on, and there are so many overloads that it's hard to get an overall picture of the API.
Suggestion:
I'm going to make 3 function templates in namespace mongo::overload: `mul`, `add`, `sub`. These match the names in the gcc builtins for these ops.
We'll just have the output parameter determine the domain type for the operation. Much easier that way:
uint64_t multiplied; - if (mongoUnsignedMultiplyOverflow64(n, base, &multiplied)) + if (overflow::mul(n, base, &multiplied)) return Status(ErrorCodes::Overflow, "Overflow"); - if (mongoUnsignedAddOverflow64(multiplied, digitValue, &n)) + if (overflow::add(multiplied, digitValue, &n)) return Status(ErrorCodes::Overflow, "Overflow");
- related to
-
SERVER-31310 Use SafeInt functions for MSVC overflow detection
-
- Closed
-