Skip to content
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

Open
wants to merge 7 commits into
base: base-2-to-the-64
Choose a base branch
from

Conversation

OmriHab
Copy link

@OmriHab OmriHab commented Apr 10, 2018

Tested partly locally, couldn't test with BigInt because I don't have all the functions needed to compile and test.
Functions changed:

  • to_string
  • operator=
  • operator+/-
  • operator>>/<<
  • operator==
  • operator<

@OmriHab
Copy link
Author

OmriHab commented Apr 10, 2018

I've updated the long long compare functions to be more efficient, that instead of building unnecessary BigInt objects and calling a lot of functions, get checked straight in the function.
Also I added a conversion to 2 uint64_t's if long long is 128 bit, unlikely but adds reliability.

Copy link
Owner

@faheel faheel left a 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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (magnitude != {0})

Copy link
Author

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.

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.
Copy link
Owner

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.

@@ -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;
Copy link
Owner

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;

}
else
return -(*this) > -num;
}
else
return sign == '-';
return is_negative;
}


Copy link
Owner

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;

@@ -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;
Copy link
Owner

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)
{ }
Copy link
Owner

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)
{ }
Copy link
Owner

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)))
Copy link
Owner

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.

std::string num_string;

for (uint64_t ull : this->magnitude)
while (ull > 0) {
Copy link
Owner

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.

@@ -8,6 +8,8 @@
#define BIG_INT_UTILITY_FUNCTIONS_HPP

#include <tuple>
#include <stdint.h>
Copy link
Owner

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>.

Copy link
Author

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::?

Copy link
Author

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

Copy link
Owner

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

@OmriHab
Copy link
Author

OmriHab commented Apr 12, 2018

Right, working on the fixes now, and thanks for the feedback!
This is my first OS contribution so sorry if it's a bit rusty.

@faheel
Copy link
Owner

faheel commented Apr 20, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants