[SERVER-43032] simplify platform/overflow_arithmetic.h Created: 25/Aug/19  Updated: 29/Oct/23  Resolved: 04/Sep/19

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: 4.3 Desired
Fix Version/s: 4.3.1

Type: Improvement Priority: Major - P3
Reporter: Billy Donahue Assignee: Billy Donahue
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-31310 Use SafeInt functions for MSVC overfl... Closed
Backwards Compatibility: Fully Compatible
Sprint: Dev Tools 2019-08-26, Dev Tools 2019-09-09
Participants:

 Description   

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");



 Comments   
Comment by Githook User [ 04/Sep/19 ]

Author:

{'name': 'Billy Donahue', 'username': 'BillyDonahue', 'email': 'billy.donahue@mongodb.com'}

Message: SERVER-43032 cast differently to fix Windows build of unit test
Branch: master
https://github.com/mongodb/mongo/commit/bfedc310559048d88747901fe62271bb1f62a90e

Comment by Githook User [ 04/Sep/19 ]

Author:

{'name': 'Billy Donahue', 'username': 'BillyDonahue', 'email': 'billy.donahue@mongodb.com'}

Message: SERVER-43032 Revise platform/overflow_arithmetic_test.cpp
Branch: master
https://github.com/mongodb/mongo/commit/947f837d67b4112dc3b4377bd01948a8e2f551b4

Comment by Billy Donahue [ 28/Aug/19 ]

update for the unit test CR https://mongodbcr.appspot.com/495150001

Comment by Githook User [ 28/Aug/19 ]

Author:

{'email': 'billy.donahue@mongodb.com', 'name': 'Billy Donahue', 'username': 'BillyDonahue'}

Message: SERVER-43032 simplify overflow_arithmetic.h
Branch: master
https://github.com/mongodb/mongo/commit/d6f11844f1c17a675c335ea95f7a49736eb653e9

Comment by Billy Donahue [ 25/Aug/19 ]

http://mongodbcr.appspot.com/503170001

Generated at Thu Feb 08 05:02:04 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.