Add log2, log10 and log256 functions#3670
Conversation
contracts/utils/math/Math.sol
Outdated
| */ | ||
| function log10(uint256 value) internal pure returns (uint256) { | ||
| uint256 result = 0; | ||
| if (value >= 10**64) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I tested both, and this change makes it more expensive :/
it looks like the compiler does the -1 operation at runtime
contracts/utils/math/Math.sol
Outdated
| if (x >> 2 > 0) { | ||
| result <<= 1; | ||
| } | ||
| uint256 result = 1 << (log2(a) / 2); |
There was a problem hiding this comment.
We can replace 1 << (log2(a) / 2) by 1 << (log2(a) >> 2).
There was a problem hiding this comment.
That is interresting, but the correct formula would be 1 << (log2(a) >> 1)
There was a problem hiding this comment.
Before changing that I would check if Solidity does not make that optimization on its own.
There was a problem hiding this comment.
tested both. using >> 1 saves 54 gas compared to / 2
(0.8.16 with optimizations)
There was a problem hiding this comment.
@Amxx Because It happen due to solidity checking underflow that. But When we are use shift operator it bypass that check.
There was a problem hiding this comment.
Overflow check make sens for +, - and * ... not for /
The only reasonable check is that we don't divide by 0 ... but there is no reason to check that here.
…com:Amxx/openzeppelin-contracts into feature/math/logX
|
Edit: Just realized that |
Yes, hex chars go by pairs. |
frangio
left a comment
frangio
left a comment
There was a problem hiding this comment.
LGTM! Looking forward to fuzzing too.
3 participants
Logs are already used in different places:
Math.sqrt, we do a log in base 4 (log2 divided by 2)String.toString(uint256), we do a log base 10String.toHexString(uint256), we do a log base 256In #3573, it was mentioned that these operation should all be in math, rather then scattered all over the code base. This rationalizes the code, and allow reuse by devs.
PR Checklist