-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated functions to vector format #25
base: base-2-to-the-64
Are you sure you want to change the base?
Conversation
I've updated the long long compare functions to be more efficient, that instead of building unnecessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (magnitude != {0})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 equal
I've also corrected the return value when the numbers are equal.
include/operators/relational.hpp
Outdated
@@ -159,7 +192,7 @@ bool BigInt::operator>(const long long& num) const { | |||
*/ | |||
|
|||
bool operator>(const long long& lhs, const BigInt& rhs) { | |||
return BigInt(lhs) > rhs; | |||
return rhs > lhs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You inverted the condition here. Should be:
return rhs < lhs;
include/operators/relational.hpp
Outdated
} | ||
else | ||
return -(*this) > -num; | ||
} | ||
else | ||
return sign == '-'; | ||
return is_negative; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -139,7 +172,7 @@ bool BigInt::operator<(const long long& num) const { | |||
*/ | |||
|
|||
bool operator<(const long long& lhs, const BigInt& rhs) { | |||
return BigInt(lhs) < rhs; | |||
return rhs < lhs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | ||
{ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as above.
@@ -39,7 +39,15 @@ BigInt::BigInt(const BigInt& num) { | |||
*/ | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -8,6 +8,8 @@ | |||
#define BIG_INT_UTILITY_FUNCTIONS_HPP | |||
|
|||
#include <tuple> | |||
#include <stdint.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to <cstdint>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in other c++ code that I've seen they don't use the std:: prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: