[trivial] Fixed in contract for Base64.decode for arrays

[ghost91-]

Forum thread: https://forum.dlang.org/thread/nnkik5$1nfq$1@digitalmars.com

[codecov-io]

Current coverage is 88.74% (diff: 100%)

Merging #4720 into master will not change coverage

@@             master      #4720   diff @@
==========================================
  Files           121        121          
  Lines         74037      74037          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          65705      65705          
  Misses         8332       8332          
  Partials          0          0          

Powered by Codecov. Last update 4a634ed...1bbe7fe

[wilzbach]

Looks reasonable to me. For the lazy people realDecodeLength:

Used in decode contracts. Calculates the actual size the decoded
result should have, taking into account trailing padding.

What about the other input contract here?
https://github.com/dlang/phobos/blob/master/std/base64.d#L1070

CC @jpf91.
Btw shouldn't base64 be in std.digest?

[ghost91-]

It does not work for the other input contract, because realDecodeLength needs to access the last two elements of the imput (https://github.com/dlang/phobos/blob/master/std/base64.d#L954). One could do this for randomaccess ranges that support opDollar, but not for all input ranges. Also, there is even an issue/enhancement request which suggests to allow decode to be used on ranges, which don't even have length() (https://issues.dlang.org/show_bug.cgi?id=9543), so we would need to drop the in contract completely in that case.

[jpf91]

Btw shouldn't base64 be in std.digest?

Could you elaborate why you think it should be in std.digest? My reasoning is std.digest is for messages digests, aka hash functions. By definition a hash function maps a variable length input to a fixed length output. base64 maps a variable length input to a variable length output and therefore is a 'encoding'.

base64 shouldn't be a top level module though, moving it to a new package std.encoding may be a good idea. std.base64 is just quite old and when it was written we didn't use subpackages in phobos yet. std.utf and std.ascii are also encodings and could be moved to std.encoding as well, AFAICS. And we also have a std.encoding module which should be part of the same package.

[wilzbach]

Also, there is even an issue/enhancement request which suggests to allow decode to be used on ranges, which don't even have length() (https://issues.dlang.org/show_bug.cgi?id=9543), so we would need to drop the in contract completely in that case.

Oh that was my question. Sorry for the confusion :/

Could you elaborate why you think it should be in std.digest?
[...]
base64 shouldn't be a top level module though,

Fair enough, I was just confused that it's in it's own top-level module.

moving it to a new package std.encoding may be a good idea. std.base64 is just quite old and when it was written we didn't use subpackages in phobos yet. std.utf and std.ascii are also encodings and could be moved to std.encoding as well, AFAICS. And we also have a std.encoding module which should be part of the same package.

👍
Do you have time to start a NG thread about this?

[jpf91]

👍
Do you have time to start a NG thread about this?

Unfortunately no, I'm quite busy with GDC and some other stuff right now :-(

[wilzbach]

Unfortunately no, I'm quite busy with GDC and some other stuff right now :-(

Alrighty -> http://forum.dlang.org/post/cobspypjiwizfrmucwyq@forum.dlang.org (I was pretty lazy).
Let's see how it goes ;-)

@ghost91 sorry for the OT discussion. Pinging due to git blame @quickfur @monarchdodra @CyberShadow.

[CyberShadow]

(Edit: removed comment due to reading fail)

• This patch only fixes the contract for the overload handling dchar arrays. Was this actually your intention? Regardless, contracts for all overloads should be consistent.
• This is missing unit tests.

[ghost91-]

Yes, it was my intention to simply fix it for dchar arrays (because this is trivial). I guess we should probably review the other contracts, too (possibly use realDecodeLength for ranges, too, when they have random acces and opDollar) and also consider the enhancement request I linked earlier.

But then, while reading through the module, I saw lots of things, which need some attention (e.g. I noticed unittests, which were obviously missing some cases; the range interaces use enforce(!empty) instead of assert in popFront and have no check in front etc...).

I might have a look at the whole module and try to fix those things, if I find the time.

I just ran into a problem with decoding of dchar arrays in a project of my own, and in this case it's trivial to fix, so I thought I'd just do that quickly. That's also the reason why I did not bother with adding additional unitests (because no funcionality is changed actually...)

[andralex]

realDecodeLength is a terrible name. What's next, definitelyActualDecodeLength? How about decodeLengthWithPadding?

[andralex]

I'll pull this because it makes no claim to fix an issue and marks a little progress. Please revisit.

[andralex]

Auto-merge toggled on

[CyberShadow]

realDecodeLength is a terrible name.

FWIW:

• The function is private
• The function has been there since the beginning
• The quantitative difference is that one function only makes an estimate based on the input length, but the other needs to look at the actual input data.