Issue 14503: Implement formatting BigInt in octal

[quickfur]

Second part of fix to: https://issues.dlang.org/show_bug.cgi?id=14503

[GassaFM]

Perhaps you meant to use the actual length here.
Otherwise, the following code gives a range check error in biguintcore.d on line 1588:

import std.array, std.bigint, std.range, std.stdio;
void main () {
    auto a = BigInt ("1" ~ ("0".repeat (10000).join ("")));
    writefln ("%o", a);
}

[GassaFM]

An assertion may be added to check that exactly 0, 1, or 2 characters of the predicted length are wasted.

[quickfur]

Good catch, I wasn't paying attention and failed to actually multiply by data.length. Fixed.

[quickfur]

Hmph, actually, assert(0 <= firstNonZero && firstNonZero <= 3); is wrong because if the BigInt has a lot of leading zeroes, firstNonZero may be quite large, up to floor(BigDigitBits/3).

[quickfur]

No worries, I fixed the problem. Hopefully it will pass now. Thanks for finding all the problems!

[quickfur]

Actually, even that assumption is not necessarily true, if the upper words of the BigInt are zero. I'm not sure if the code will ever generate such a BigInt, but the assert will trigger if this ever happens. I think it's safer to just take out the assert.

[GassaFM]

With the current assertion, the following passes now (32-bit):

    BigInt c = 1;
    foreach (step; 0..1000) {
        writefln ("%o", c);
        c *= 2;
    }

So perhaps the statement is right.

[quickfur]

Maybe try c /= 2 after your loop to see if the code ever produces a BigInt with zeroes in the upper words?

[GassaFM]

Yeah, if "no leading zero words" is not explicitly guaranteed somewhere, the assertion is still wrong and should be taken out.

[GassaFM]

A descending loop also works fine:

    foreach (step; 0..1000)
    {
        writefln ("%o", c);
        c /= 2;
    }

But who knows what else won't work :) .

If leading zero words ever appear, or will appear, this particular assertion would be a wrong place to signal that anyway.

[quickfur]

I glanced through the various operations, and it seems that a lot (all?) of them allocate new arrays for storing the result of operations. I'm not sure how accurate the array size estimates are, whether there might be leading zero words or not. So better leave the assert out.

[quickfur]

Hmm, autotester is failing... I wonder if there's some problems with the length prediction code??

[GassaFM]

Found while trying to test this PR: https://issues.dlang.org/show_bug.cgi?id=15678

[quickfur]

Fixed: #3995

[quickfur]

Rebased.

#3995 has been merged; any other issues?

[DmitryOlshansky]

LGTM

[quickfur]

ping random reviewers @andralex @JakobOvrum @schveiguy @klickverbot @DmitryOlshansky
This has been sitting here for a while. Is it a go or a no-go?

[schveiguy]

Why is this accepting int? Every call to this is with an unsigned type, and most of them are size_t.

May be a good reason, but I feel like this should be size_t or ptrdiff_t -- i.e. whatever is natural for the architecture.

[quickfur]

You're right, this should not accept a signed integer type. I refrained from using size_t because the range of parameters should always be < 8 (otherwise there is a bug). No need to use 64 bits when 32 bits already suffices (in fact, just 4 bits already suffice, but requiring anything smaller than 32 bits requires ugly casts and doesn't necessarily make the generated asm any better).

[schveiguy]

Implementation looks sound. Is there a reason you didn't implement fromOctalString?

[quickfur]

I'm leaving fromOctalString to a followup PR because I wanted to avoid merge conflicts with #3876 .

[schveiguy]

Auto-merge toggled on