Issue 15678: BigInt("") should not break array bounds

[quickfur]

Fixes: https://issues.dlang.org/show_bug.cgi?id=15678

[quickfur]

Decided to just initialize a zero BigInt instead of throwing or asserting, as I didn't want to introduce possible breakage in nothrow code, and the original bug report was complaining about array overrun in -release (no-assert) mode.

[kyllingstad]

In my opinion, it would have been better if the constructor threw, but I see your point about not wanting to break nothrow code. This is a drawback of deduced function attributes that I haven't thought of before. Interestingly, it seems that someone have already planned to make the constructor throw in some cases, cf. the comment a few lines up.

[kyllingstad]

Also, if we define BigInt("") to be zero I guess it should be documented.

[quickfur]

I noticed the comment too, but wasn't sure whether to actually make the constructor throwing. It does date from quite a few years back, and things have changed since.

Anyway, updated the ddoc comment. I changed it from /// style to /** */ style because for some strange reason dmd git HEAD seems to treat every line in /// style as a separate paragraph, whereas this particular comment really would look better with multiple sentences combined in a single paragraph.

[GassaFM]

I have little experience with nothrow, so I'd definitely make it throw.
What happens if I call BigInt ("ONE"), or BigInt ("qwerty"), does it not throw anyway?..

I see BigInt (string) as a counterpart to std.conv.to !(int) (string), which throws if something is wrong, and rightfully so.

Moreover, if the constructor throws, an important semantic invariant like text (BigInt (s)) == s will hold.
Hmm, or will it additionally convert "-0" to "0"?

[GassaFM]

Well, got my hands on a D compiler now to answer my own questions.

1. auto a = BigInt ("ONE"); throws:
   std.conv.ConvException@std\internal\math\biguintcore.d(1606): invalid digit
2. -0 is silently converted to 0, which may be a good thing for a general-purpose library.

[GassaFM]

Point 1 shows that the constructor is not nothrow anyway, so this is not a concern.

As to point 2, it already provides an undocumented exception to the semantic invariant.
Furthermore, for example, "00" also becomes "0".

I should note that converting "" to "0" is less innocent than converting "-0" or "00" to "0": additionally, it might break some tokenization routines when converting back and forth.

At any rate, I don't see a use case for silently handling "" in a defined way instead of issuing a prominent error.
But if you do, I'm fine with that.

[kyllingstad]

@GassaFM: You are absolutely right, thanks for checking this! It seems that BigUint.fromHexString() is nothrow while BigUint.fromDecimalString() throws. Here are lines 398–340 of biguintcore.d (and note the comments):

// return true if OK; false if erroneous characters found
// FIXME: actually throws `ConvException` on error.
bool fromDecimalString(const(char)[] s) pure @trusted

It seems that this needs some more cleaning-up. Now I definitely think that the constructor should throw, and it should throw the same exception whether something is an empty string, an invalid hex string or an invalid decimal string.

[GassaFM]

From another point of view:

How can a BigInt (range-of-char) constructor be made nothrow or @nogc in principle?
At the core, it should allocate an unknown amount of memory for storage, which I'm sure can result in OutOfMemoryError or something similar.
Supplying a pre-allocated buffer as argument does not immediately help - what if the length is not enough?

[quickfur]

Modified to throw exception on empty string instead.

[quickfur]

Also changed current code to throw on invalid digit string.

[quickfur]

@GassaFM OutOfMemory errors, AFAIK, are Errors, not Exceptions, so they can be thrown even in nothrow code, and are generally fatal, just as AssertErrors are fatal. But in general, I agree that the ctor can't be nothrow, because there's no sane way of aborting construction of a BigInt when given a garbage input string. Except perhaps setting it to 0, which is probably a bad idea as it will hide bugs.

[quickfur]

Weird autotester failure, doesn't seem related to this PR? Am I missing something here?

[quickfur]

OK, seems to be passing now.

[quickfur]

ping @kyllingstad
Any other comments? Is this ready to merge?

[kyllingstad]

Auto-merge toggled on

[kyllingstad]

Sorry, slipped my mind :-/

[quickfur]

No problem, thanks!