BitArray/BitFields - Review

[Dmitry Olshansky]

On 29-Jul-12 00:41, Era Scarecrow wrote:
>
> BitFields:
> https://github.com/rtcvb32/phobos/commit/729c96e2f20ddd0c05a1afb488030b5c4db3e012
>
>
> new BitArray Overhead functions/templates:
> https://github.com/rtcvb32/phobos/commit/55aaf82a39f3901f03f5f1ac26fd175c5b2cd167
>
>
> BitArray:
> https://github.com/rtcvb32/phobos/commit/568c0d1966208afe70cedcaddc3bb880dc3d481d
>
>
>   I'd like opinions of the code I've submitted, and certain shortcomings
> that are present that appropriate changes would be needed to fix. Only
> one of these has been a problem during debugging and that's due to
> references. (All three patches must be present to work)
>

Great! But I strongly suggest to repost it in d.D newsgroup as it has
wider audience and is more appropriate for reviews.

> BitArray:
> 1) Slices use references (sometimes) to previous bitarray. Since it's a
> slice (like an array) this could be expected.
> Solutions:
>   a) Drop the union, make all functions @safe (compared to @trusted),
> and they are all ref/slices
>   b) Leave as sometimes ref, sometimes not; Use .dup when you NEED to
> ensure different unique copies.
>   c) Apply COW (Copy-On-Write) where a known slice type (canExpand is
> false) when a change would apply, it replaces the present slice into
> it's own unique array.
>   d) All slices become new dup'ed allocated arrays (Most expensive of
> them all)

My solution is make slices different type e.g. BitSlice.
It's always reference to the original BitArray. Array itself still can 
be either compact or not. All operations with slice go to array (and 
still check if they can use word-aligned speed up).

How does it sound?

>
> 2) opCast disabled: Anyone using opCast their code will break, however
> depending on it's usage that may not be an issue. If anyone can
> recommend alternative opCast(s) that won't prevent normal use of
> casting, then they will be reimplemented.
>
> 3) dim - An artifact of the previous BitArray, and I think needs to be
> removed as it has limited use.
>
> 4) Certain operators (opCat, opCat_r) I can't replace with
> opBinary/opBinary_r.
>

opCat isn't it just operator "~" and "~=" ? You can use opOpAssign for 
"~=" IRC.

> 5) Examples of additional use cases/construction that it currently fails
> and should work.
>
> 6) Added rawWrite (for primitives), usefulness testing and opinions.
>
> 7) opApply, toHash, opCmp, opEqual tested in various environments (Like
> associative arrays) for problems, along with any possible const issues.
>
> BitFields:
>   I've concentrated on adding default values. A set of functions
> previously present string functions 'myXXX' were present, why were they
> present to begin with?

I suspect to!string wasn't CTFEable. Regardless you can catch a word 
from Andrei Alexandrescu on the main D newsgroup who (I think) came up 
with it in the first place.


-- 
Dmitry Olshansky