Updated functions to vector format#25
Updated functions to vector format#25OmriHab wants to merge 7 commits intofaheel:base-2-to-the-64from
Conversation
|
I've updated the long long compare functions to be more efficient, that instead of building unnecessary |
faheel
left a comment
There was a problem hiding this comment.
@OmriHab Thanks a lot for these changes! I have reviewed all the changes except those in functions/utility.hpp and operators/binary_arithmetic.hpp. Will review them soon.
There's a suggestion I'd like to give about non-fixed integer types. Instead of checking that their size matches 64 bits, I suggest we change all occurrences of long long to int64_t and unsigned long long to uint64_t.
| } | ||
| temp.magnitude = magnitude; | ||
| // If magnitude is not 0 | ||
| if (magnitude.size() != 1 and magnitude[0] != 0) |
There was a problem hiding this comment.
Can't do that in c++, it's possible to do if (magnitude != std::vector<uint64_t>(1,0)) but I think it's better to compare them like this.
include/operators/relational.hpp
Outdated
| if (is_negative == num.is_negative) { | ||
| if (not is_negative) { | ||
| if (magnitude.size() == num.magnitude.size()) { | ||
| // Check in reverse order, magnitudes are saved LSB first. |
There was a problem hiding this comment.
This can be improved by using minimal amount of nested conditions:
if (is_negative != num.is_negative) // signs are opposite
return is_negative;
if (is_negative) // both numbers are negative
return -(*this) > -num;
// both numbers are positive
if (magnitude.size() != num.magnitude.size())
return magnitude.size() < num.magnitude.size();
// both magnitudes have the same number of digits
// compare their digits, from MSD to LSD
for (int64_t i = magnitude.size() - 1; i >= 0; i--)
if (magnitude[i] != num.magnitude[i])
return magnitude[i] < num.magnitude[i];
return false; // both numbers are equalI've also corrected the return value when the numbers are equal.
include/operators/relational.hpp
Outdated
|
|
||
| bool operator>(const long long& lhs, const BigInt& rhs) { | ||
| return BigInt(lhs) > rhs; | ||
| return rhs > lhs; |
There was a problem hiding this comment.
You inverted the condition here. Should be:
return rhs < lhs;| return is_negative; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
I realised that the code for BigInt > BigInt (in the lines that follow this) can be simplified to:
return num < *this;
include/operators/relational.hpp
Outdated
|
|
||
| bool operator<(const long long& lhs, const BigInt& rhs) { | ||
| return BigInt(lhs) < rhs; | ||
| return rhs < lhs; |
There was a problem hiding this comment.
You inverted the condition here. Should be:
return rhs > lhs;| BigInt::BigInt() | ||
| : magnitude(1,0) | ||
| , is_negative(false) | ||
| { } |
There was a problem hiding this comment.
Lines shouldn't start with a grammar symbol (like commas and colons):
BigInt::BigInt():
magnitude(1, 0),
is_negative(false) { }| BigInt::BigInt(const BigInt& num) | ||
| : magnitude(num.magnitude) | ||
| , is_negative(num.is_negative) | ||
| { } |
|
|
||
| BigInt::BigInt(const long long& num) { | ||
| magnitude = { (unsigned long long) llabs(num) }; | ||
| if (sizeof(long long) == (sizeof(uint64_t))) |
There was a problem hiding this comment.
Instead of these checks at multiple places, I suggest we change all occurrences of long long to int64_t and unsigned long long to uint64_t.
include/functions/conversion.hpp
Outdated
| std::string num_string; | ||
|
|
||
| for (uint64_t ull : this->magnitude) | ||
| while (ull > 0) { |
There was a problem hiding this comment.
This wouldn't work. Base conversion will need to be done. I'm working on it, so you can leave this as it previously was.
include/functions/utility.hpp
Outdated
| #define BIG_INT_UTILITY_FUNCTIONS_HPP | ||
|
|
||
| #include <tuple> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
Correct me if i'm wrong, would that require every reference to uint64_t to be prefixed with std::?
There was a problem hiding this comment.
Because in other c++ code that I've seen they don't use the std:: prefix
There was a problem hiding this comment.
No, you wouldn't need the std:: namespace for int64_t and uint64_t
|
Right, working on the fixes now, and thanks for the feedback! |
|
@OmriHab Just wanted to let you know that I've been working on the conversion part (decimal string ↔ base 264 vector) and it's almost done. I'll push the changes in a while and you then update your PR with those. I'll also review the remaining changes in your PR once I push the changes. |
Tested partly locally, couldn't test with BigInt because I don't have all the functions needed to compile and test.
Functions changed: