Fixed issue #224: ion_int_from_long doesn't return #225
Conversation
Addressed a corner case in ion_int_from_long that caused an infinite loop when MIN_INT64 was passed in as the long value.
|
The fix looks good, but can you please include the unit test? |
Polishing up my unit test has surfaced another problem. 😞 See #226. I'm going to try and roll a fix for that into this PR. |
* Modified the `ion_int_to_int64` function to work on 63- and 64-bit magnitudes. * Adds unit roundtripping unit tests that convert an int64_t to an IINT and then back to an int64_t again.
| // iint's magnitude is stored in an array of 31-bit II_DIGIT values. Magnitudes that | ||
| // require 63 or 64 bits to represent will then take 3 II_DIGITS, only using 1 to 2 | ||
| // bits from the most significant II_DIGIT. | ||
| const uint32_t max_digits_per_int64_t = 3; | ||
| const uint32_t whole_digits_per_int64_t = 2; | ||
| const uint32_t max_partial_digit_value = 3; // == 0b11, two populated bits |
There was a problem hiding this comment.
These constants went through a few iterations before I settled on this.
I tried re-calculating these values based on sizeof(int64_t) and II_BITS_PER_II_DIGIT, but it was pretty verbose:
const uint32_t bits_per_int64_t = sizeof(int64_t) * II_BITS_PER_BYTE;
const float digits_per_int64_t = (float) bits_per_int64_t / (float) II_BITS_PER_II_DIGIT;
const uint32_t max_digits = ceilf(digits_per_int64_t);
const uint32_t whole_digits = floorf(digits_per_int64_t);
const uint32_t max_bits_in_partial_digit = bits_per_int64_t % II_BITS_PER_II_DIGIT;
const uint32_t max_partial_digit_value = (1<<max_bits_in_partial_digit) - 1;While a compiler would've probably been able to reduce these to zero-cost constants at build time, the complexity it introduced for folks trying to read the code felt too expensive.
Given that we're using a type name that will always be 64-bits (int64_t instead of, say, long), this would only ever need to be reevaluated if the definition of II_BITS_PER_II_DIGIT (currently 31) ever changed, which won't happen any time soon. The code above didn't seem to carry its own weight. I opted to just use the resulting values instead.
I also tried defining macros for these values in ion_int.h, but it felt like overkill; readers would have to track down their definitions in another file despite this being the only place they're ever used.
| if (is_negative) { | ||
| // This negates an unsigned value, which is well-defined behavior. Doing so handles the | ||
| // MIN_INT64 case: an int64_t whose absolute value is too large to be stored in an int64_t. | ||
| magnitude = -magnitude; |
There was a problem hiding this comment.
Here's a helpful StackOverflow answer that includes the relevant text of the C spec.
| // unsigned to accommodate the absolute value of MIN_INT64, which requires 64 bits to store. | ||
| uint64_t magnitude; | ||
| // Used for shifting operations that consume the variable. | ||
| uint64_t temp_magnitude; |
There was a problem hiding this comment.
magnitude is bit-shifted to zero twice. Given that it takes a small amount of expense to calculate it the first time, I introduced this temporary variable that could be reset to the value of magnitude between bit-shifting operations.
Using the modern for-each looping syntax caused the Linux/GCC 4.9 CI build to fail.
* Switched from `ion_int_init` to `ion_int_alloc` for Ion integer initialization in the unit tests. * Added a unit test to verify that `ion_int_to_int64` detects overflow as expected. It tests both the case where the value can fit in 64 unsigned bits (but not 64 signed bits) and the case where the number is too big to fit in 64 bits.
This PR addresses a corner case in
ion_int_from_longthat would cause an infinite loop when MIN_INT64 is passed in as the long value. A detailed description of the problem can be found in issue #224.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.