Possible bug in BigInteger to/from MemoryBlock

General JUCE discussion
1

I’ve been working on writing unit tests for my implementations of VariantConverters for various juce types, and I think I’ve found a bug in BigInteger’s to/from MemoryBlock functions.

First, here’s what the unit test code looks like:

void runTest() final 
{
    BigInteger orig { getRandom().nextInt64() };

    const auto var     = VariantConverter<BigInteger>::toVar (orig);
    const auto decoded = VariantConverter<BigInteger>::fromVar (var);
        
    expect (decoded == orig);
}

At first, my VariantConverter<BigInteger> implementation looked like this:

template <>
struct VariantConverter<BigInteger>
{
	static BigInteger fromVar (const var& v)
    {
        BigInteger i;

	    if (const auto* block = v.getBinaryData())
		    i.loadFromMemoryBlock (*block);

	    return i;
    }

	static var toVar (const BigInteger& i)
    {
        return { i.toMemoryBlock() };
    }
};

With this implementation, my unit test occasionally succeeded but sometimes spuriously failed; upon inspecting the failure conditions, it seems like the decoded BigInteger has the same values as the original, but the opposite sign.

So I changed my VariantConverter to this:

template <>
struct VariantConverter<BigInteger>
{
	static BigInteger fromVar (const var& v)
    {
        BigInteger i;
    
        if (const auto* obj = v.getDynamicObject())
        {
            if (obj->hasProperty (data))
                if (const auto* block = obj->getProperty (data).getBinaryData())
                    i.loadFromMemoryBlock (*block);
        
            if (obj->hasProperty (negative))
                i.setNegative ((bool) obj->getProperty (negative));
        }

	    return i;
    }

	static var toVar (const BigInteger& i)
    {
        DynamicObject obj;
    
        obj.setProperty (data, i.toMemoryBlock());
        obj.setProperty (negative, i.isNegative());
    
	    return { obj.clone().get() };
    }

private:
    static constexpr auto data = "Data";
    static constexpr auto negative = "Negative";
};

And now the test succeeds reliably.

1 Like
2

From https://docs.juce.com/master/classBigInteger.html:

Negative values are possible, but the value isn’t stored as 2s-complement, so be careful if you use negative values and look at the values of individual bits.

So it appears the sign isn’t captured in the binary representation, therefore storing it as an additional property is the way to go in your case.

2 Likes
3

but the value isn’t stored as 2s-complement,

That makes sense (because of the variable length), but it isn’t obvious.
From the method name “toMemoryBlock()” I would expect to store anything.

Maybe the name should be renamed to reflect that behaviour, like “exportUnsignedValueToMemoryBlock” I guess there are better names for this :wink:

2 Likes
4

yeah, I remember reading the bit about “it’s not stored as 2’s complement” in the docs, but any type that has a toMemoryBlock() and fromMemoryBlock() function, I would expect to restore entirely/perfectly when converting to MemoryBlock and back again.

Is there any reason why the sign can’t be stored in the MB as well? It’s just one additional boolean bit of information…

5

Is there any reason why the sign can’t be stored in the MB as well? It’s just one additional boolean bit of information…

The problem is, you never know for what the first bit stands for, is it the sign, or just power of two?. 2s.complement is only possible for fixed size integers. You may could use the last bit, but this will break compatibility.

1 Like
6

I see…

7

It is probably best to view the BigInteger not as a number but as a std::bitset.
I think the only chance for a signed BigInteger is adding an extra flag in a struct:

struct SignedBigInteger
{
    juce::BigInteger number;
    bool             negative; // sign
};

But now you also need to overload the maths to flip the bits if sign is negative
That is a can of worms IMHO.

8

you miss the point, BigInteger has already a sign flag, and uses it internally

9

Oh, I missed it indeed.
Sorry for the noise and thanks for correcting.

Si tacuisses…