[WIP] std.range: implement bitwise adapter over Integral type Ranges by edi33416 · Pull Request #4927 · dlang/phobos

edi33416 · 2016-11-30T13:55:38Z

Add a generic bitwise adapter to std.range

jmdavis · 2016-11-30T19:11:39Z

I don't have time to look at this in depth right now, but I would point out that it would be more appropriate for something like this to be in std.bitmanip.

andralex · 2016-12-05T22:12:31Z

@jmdavis I was joking to @edi33416 that an older (Google?) paper argued that tag-based systems are better and more intuitive to humans than hierarchical systems. This seems to be a good anecdote supporting that. Bitwise defines a range interface on top of a range interface, so it's about std.range. It also does a bunch of bit-level access, so it's about std.bitmanip. We have a hierarchical package system so whatever we choose won't be the perfect fit.

jmdavis · 2016-12-06T01:02:37Z

@andralex Well, since a large portion of Phobos is just code that creates ranges of one sort or another (std.algorithm most notably), if the fact that something is a range puts it in std.range, a large portion of Phobos belongs in there. Obviously, it's sometimes non-obvious or debatable where something should go, but it seems that std.range has mostly been for range primitives and stuff that are building blocks for ranges, which this isn't. std.bitmanip, on the other hand, is where all of the bit manipulation / bitwise stuff has gone thus far. And since that's the sort of thing that this is doing, I think that it makes a lot more sense for it to be in std.bitmanip. So, while it's certainly debatable, I would have thought that it was fairly clear in this case, and I'm inclined to resist putting stuff in std.range just because it involves a range, particularly since ranges are so pervasive in Phobos and only becoming more so.

andralex

Great work, ready for one more pass with an eye for minimalism.

andralex · 2016-12-05T22:13:16Z

std/range/package.d

 }

+/**
+Bitwise adapter over Integral type Ranges. Consumes the range elements bit by bit.


Integral and Ranges in lowercase

andralex · 2016-12-05T22:14:55Z

std/range/package.d

+    if (isInputRange!R && isIntegral!(ElementType!R))
+{
+    alias ER = ElementType!R;
+    alias UER = Unsigned!ER;


We try to avoid acronyms that lead to multi-capitalized names. I suggest

private alias ElementType = ElementType!R; private alias MaskType = Unsigned!ElementType;

andralex · 2016-12-05T22:17:52Z

std/range/package.d

+
+    private R parent;
+    private UER mask;
+    private int maskPos;


prefer uint for this kind of stuff

andralex · 2016-12-06T02:34:58Z

std/range/package.d

+
+    private enum UER bitsNum = ER.sizeof * 8;
+
+    this()(auto ref R range)


Pass by value should suffice here, no?

I used a pass by ref here in order to copy the range once instead of twice (first when it gets passed to the ctor and the second time when it gets copied into parent)

The compiler should be eliding the copies anyway. You shouldn't need to worry about it.

andralex · 2016-12-06T02:37:23Z

std/range/package.d

+        }
+    }
+
+    bool empty() const


If you put const on this method you force parent.empty to also be const. If you leave no const, deduction will take care of it appropriately (making it const wherever possible).

Unless something has changed recently, const is not inferred. So, if you want it to be const or not based on the template argument, then you have to use introspection and static if. That being said, it still shouldn't just be marked as const, because then it won't work with many ranges.

const inferrence does have a workaround:

bool empty(this This)() { ... }

This will only compile on a const wrapper range if all the calls can be made on the lower range. It's more crude than defining or not defining the method as const, but it would work.

However, my opinion is that const on ranges is useless anyway -- you can't iterate a const range. So I would recommend just not putting const here.

andralex · 2016-12-06T02:47:22Z

std/range/package.d

+    private void initPrivates()
+    {
+        // Helper function to avoid code duplication
+        mask = cast(UER)(1) << (bitsNum - 1);


UER(1) should suffice

andralex · 2016-12-06T02:49:51Z

std/range/package.d

+    {
+        typeof(this) save()
+        {
+            return this;


Here you need to save parent as well:

typeof(this) save() { auto result = this; result.parent = parent.save; return result; }

andralex · 2016-12-06T02:51:43Z

std/range/package.d

+
+        bool back()
+        {
+            return cast(bool)(backAccumulator & backMask);


to avoid using cast and triggering all greps for unsafe code, use return (backAccumulator & backMask) != 0;

Also, here you can use const on the method because you don't depend on any templated stuff.

andralex · 2016-12-06T02:52:45Z

std/range/package.d

+            }
+            else
+            {
+                mask <<= 1;


Is that mask or backMask?

andralex · 2016-12-06T02:53:12Z

std/range/package.d

+                import core.exception : RangeError;
+                import std.exception : enforce;
+
+                enforce(n < len, new RangeError);


Inside contracts always use assert, never use enforce

andralex · 2016-12-06T02:56:37Z

@jmdavis how about this: a close review of the next round of this code will amplify your voting rights by 10x :)

andralex · 2016-12-06T02:57:22Z

(BTW I approve the feature.)

jmdavis · 2016-12-06T03:25:30Z

@jmdavis how about this: a close review of the next round of this code will amplify your voting rights by 10x :)

LOL. Okay.

9il · 2016-12-07T09:37:18Z

+1 for @jmdavis comment about std.bitmanip

9il · 2016-12-07T09:41:36Z

Also, if we add this, what is real world use case?

9il · 2016-12-07T10:02:22Z

I asked because range API is not universal. And maybe use cases you will find may require more specialised API. Note, that we do not need this for RNGs because it can be replaced with combination of Bernoulli2Variable and universal range adaptor.

This is good example where more generic != better.

If one need a lazy by element, then he probably needs to be able to get a multiple bits at once.

In other hand, if one need a random access binary range, an ndslice based implementation requires only 30 LOC. ndslice is preferable if you need a random access binary range. Furthermore ndslice based implementation would be faster (does not requires any if conditions)

It looks better for me understand use cases and, if we have them, add adaptors to existing one API instead of introducing a new large piece of code.

9il · 2016-12-07T10:37:12Z

Oops, we already have BitRange :-( http://dlang.org/phobos/core_bitop.html#.BitRange
It should be deprecated it returns indexes

9il · 2016-12-07T11:01:39Z

Proof of concept.
It can be generalized easily for common random access range if required

struct BitMap
{
    size_t* ptr;

    import core.bitop;

    bool opIndex(size_t index) const
    {
        return bt(ptr, index) != 0;
    }

    void opIndexAssign(bool val, size_t index)
    {
        if(val)
            bts(ptr, index);
        else
            btr(ptr, index);
    }
}

import std.experimental.ndslice;

void main()
{
    auto arr = new size_t[3];
    auto sl = BitMap(arr.ptr).sliced(size_t.sizeof * 8 * arr.length);

    sl[4] = true;
    sl[100] = true;
    sl.popFrontN(3);
    assert(sl[1]);
    assert(sl[97]);
}

9il · 2016-12-07T11:40:33Z

Forum thread: http://forum.dlang.org/post/vnhmrfjjennoytgomkfs@forum.dlang.org

edi33416 · 2016-12-09T17:45:38Z

std/range/package.d

-            {
-                elemIndex = 0;
-            }
+            size_t elemIndex = n / bitsNum + 1;


Do you think that the following construct would be faster?
elemIndex = n >> (bitsNum.bsf + 1) + 1;

edi33416 · 2016-12-09T17:46:28Z

This is still a WIP

andralex

Let's drop the length cache, plus a few nits

andralex · 2016-12-10T15:21:31Z

std/range/package.d

+
+    static if (hasLength!R)
+    {
+        ulong len;


Let's delete this field and compute it on the fly from the source's length and the mask(s). It looks like more work but it's simpler and may actually be faster.

andralex · 2016-12-10T15:23:34Z

std/range/package.d

+        }
+    }
+
+    bool empty()


Add a ddoc comment before this

andralex · 2016-12-10T15:24:08Z

std/range/package.d

+    bool front()
+    {
+        assert(!empty());
+


no need for all this whitespace - just say what you mean and be done

andralex · 2016-12-10T15:24:52Z

std/range/package.d

+            parent.popFront;
+            maskPos = bitsNum;
+        }
+


too much whitespace - in Phobos we don't use whitespace around each compound statement

andralex · 2016-12-10T15:25:04Z

std/range/package.d

+
+    static if (hasLength!R)
+    {
+        ulong length() const


this needs to return size_t

andralex · 2016-12-10T15:26:39Z

std/range/package.d

+            static if (hasLength!R)
+            {
+                assert(n < len,
+                       __PRETTY_FUNCTION__ ~ ": Index out of bounds");


keep on same line

andralex · 2016-12-10T15:27:18Z

std/range/package.d

+               By truncating n with maskPos bits we have skipped the remaining
+               maskPos bits in parent[0], so we need to add 1 to elemIndex.
+             */
+            size_t elemIndex = n / bitsNum + 1;


could be const/immutable

andralex · 2016-12-10T15:28:41Z

std/range/package.d

+        {
+            size_t sliceLen = end - start;
+            size_t startElemIndex;
+            size_t endElemIndex;


move down closest to initialization point

andralex · 2016-12-10T15:28:58Z

std/range/package.d

+            }
+
+            // Get the slice to be returned from the parent
+            R resultParent = (parent.save)[startElemIndex .. endElemIndex + 1];


no need for save when indexing or slicing

andralex · 2016-12-10T15:29:43Z

std/range/package.d

+
+            typeof(return) result;
+
+            result.parent = resultParent;


edi33416 · 2016-12-13T15:02:37Z

With this change, I have:

removed duplicate unittests
removed branching from opIndex and opSlice by using bitwise operations
reduced vertical whitespace

… ulong types

andralex

cool cool cool

andralex · 2016-12-19T00:19:18Z

std/range/package.d

+    R = an input range to iterate over
+
+Returns:
+    At minimum, an input range. If `R` was a forward, bidirectional or random


How about:

A Bitwise input range with propagated forward, bidirectional and random access capabilities

andralex · 2016-12-19T00:21:09Z

std/range/package.d

+    {
+        static if (hasLength!R)
+        {
+            return length() == 0;


prevalent style is to omit () on no-argument calls

andralex · 2016-12-19T00:23:59Z

std/range/package.d

+    {
+        bool back()
+        {
+            assert(!empty());


this function is not covered (please install the Codecov Chrome Extension and you'll see uncovered lines in red on this page)

andralex · 2016-12-19T00:24:14Z

std/range/package.d

+
+        void popBack()
+        {
+            ++backMaskPos;


assert(!empty) I guess

andralex · 2016-12-19T00:25:08Z

std/range/package.d

+        }
+
+        /**
+          Assignes `flag` to the `n`th bit within the range


s/Assignes/Assigns/

andralex · 2016-12-19T00:25:46Z

std/range/package.d

+
+        Bitwise!R opSlice()
+        {
+            return this;


Add .save

wilzbach

(a couple of nits)

wilzbach · 2016-12-19T06:31:24Z

std/range/package.d

+             */
+            static if (hasLength!R)
+            {
+                assert(n < length(), __PRETTY_FUNCTION__ ~ ": Index out of bounds");


While __PRETTY_FUNCTION__ is nice, I think there was some consensus to just use plain/old asserts for range asserts.

wilzbach · 2016-12-19T06:34:08Z

std/range/package.d

+// Test opIndex and opSlice
+///
+@system unittest
+{


/// will render this as example on the public docs and we don't want to shock the users by exposing all intrinsic tests in the example.
Could you please add a nice example with one type for the public docs?

wilzbach · 2016-12-19T06:34:36Z

std/range/package.d

+            assert(bw.empty());
+            static if (isForwardRange!T)
+            {
+                assert(bw2.empty());


ditto about not using parenthesis with no args

wilzbach · 2016-12-19T06:35:35Z

std/range/package.d

+Bitwise adapter over integral type ranges. Consumes the range elements bit by bit.
+
+Params:
+    R = an input range to iterate over


It's probably better to be a bit more precise here -> `integral input range"

wilzbach · 2016-12-19T06:36:46Z

std/range/package.d

+    R = an input range to iterate over
+
+Returns:
+    At minimum, an input range. If `R` was a forward, bidirectional or random


How about:

A Bitwise input range with propagated forward, bidirectional and random access capabilities

wilzbach · 2016-12-19T06:55:09Z

std/range/package.d

+        assert(bw[1] == true);
+
+        auto bw2 = bw[0 .. $ - 5];
+        auto bw3 = bw2[];


unittest blocks in D are cheap and help the reader to show that there's a new context.
Also it has happened quite often that one accidentally has introduced errors by mistyping the variable end number e, g. 4 vs 5.

wilzbach · 2016-12-19T06:57:19Z

std/range/package.d

+}
+
+// Test all range types over all integral types
+///


This is an internal test - so it shouldn't be exposed to the user. Rather provide small, simple "snippets" that can be quickly grasped (e.g. five lines).

wilzbach · 2016-12-19T06:59:08Z

std/range/package.d

+            static if (isBidirectionalRange!T)
+            {
+                auto bw3 = bw.save;
+                auto bw4 = bw.save;


Cryptic naming like bw3, bw4 make it very hard to read your code :/
How about bwTestBack?

wilzbach · 2016-12-19T07:03:47Z

std/range/package.d

+
+// Test all range types over all integral types
+///
+@safe unittest


with the enforce removed this should be possible as pure nothrow @safe

wilzbach · 2016-12-19T07:10:58Z

std/range/package.d

+    bool front()
+    {
+        assert(!empty());
+        return (parent.front & mask(maskPos)) != 0;


Calculating the mask every time seems to be an unnecessary expensive operation. What is your rationale for storing the position and not the mask directly?
(front is expected to be called a lot more often thanopSlice and opIndex`)

wilzbach · 2016-12-19T07:16:43Z

btw @edi33416 you might be interested in reviving the bits printer of #4703

andralex · 2016-12-19T19:23:23Z

Auto-merge toggled on

wilzbach · 2016-12-19T19:26:13Z

Auto-merge toggled on

@andralex did you see @9il's discussion about adding more code bloat?

http://forum.dlang.org/post/vnhmrfjjennoytgomkfs@forum.dlang.org

9il · 2016-12-19T19:28:20Z

@andralex did you see @9il's discussion about adding more code bloat?

http://forum.dlang.org/post/vnhmrfjjennoytgomkfs@forum.dlang.org

Yes, but we deprecated ndslice

wilzbach · 2016-12-20T07:41:54Z

@edi33416 would you be so kind to add a changelog entry, s.t. everyone sees your great work on the next release?

wilzbach · 2016-12-22T04:11:39Z

Btw did anyone knew about BitRange being part of Druntime since this summer?

andralex · 2016-12-23T19:08:20Z

@wilzbach yah, it's fairly specialized (for the GC) though.

MartinNowak · 2016-12-25T11:06:18Z

It's unfortunate that Ilya's point wasn't picked up.
http://forum.dlang.org/post/vnhmrfjjennoytgomkfs@forum.dlang.org
This is fairly redundant with BitArray.opApply and core.bitop.BitRange, please reconsider adding it to std.range in it's current form.

MartinNowak · 2016-12-25T11:11:58Z

@wilzbach yah, it's fairly specialized (for the GC) though.

BitRange is a range over the 1-bit indices of an integral array.

9il added @andralex Review:Needs Decision and removed Review:Needs Decision labels Nov 30, 2016

andralex requested changes Dec 6, 2016

View reviewed changes

andralex added Review:Needs Work and removed @andralex labels Dec 6, 2016

edi33416 force-pushed the bitwise_adapter branch from 1684929 to c3e620e Compare December 9, 2016 17:36

edi33416 commented Dec 9, 2016

View reviewed changes

andralex requested changes Dec 10, 2016

View reviewed changes

edi33416 added 6 commits December 15, 2016 17:51

std.range: implement bitwise adapter over Integral type Ranges

0b7cb16

Fixed cast bug that caused the bitmask to be a 32bit int for long and…

3209adf

… ulong types

Addressed comments. Refactored code. Fixed bugs.

f5c1e47

Make unittest assertThrown catch Error

7aff207

Remove branching from opIndex and opSlice using bitwise ops

37b4063

Add ddocs

5faa96d

edi33416 force-pushed the bitwise_adapter branch from 5f498cc to 5faa96d Compare December 15, 2016 15:51

andralex approved these changes Dec 19, 2016

View reviewed changes

wilzbach reviewed Dec 19, 2016

View reviewed changes

wilzbach added the pending changelog label Dec 19, 2016

andralex merged commit b6bcf75 into dlang:master Dec 19, 2016

wilzbach mentioned this pull request Dec 21, 2016

First cleanup of bitwise adapter + add to docs #4981

Merged

wilzbach mentioned this pull request Dec 25, 2016

Revert "[DEMO for DIP1005] Converted imports to selective imports in std.array" #4964

Merged

edi33416 deleted the bitwise_adapter branch October 9, 2017 14:16


		private enum UER bitsNum = ER.sizeof * 8;

		this()(auto ref R range)

Uh oh!

Comments

Conversation

edi33416 commented Nov 30, 2016

Uh oh!

jmdavis commented Nov 30, 2016

Uh oh!

andralex commented Dec 5, 2016

Uh oh!

jmdavis commented Dec 6, 2016

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Dec 6, 2016

Uh oh!

andralex commented Dec 6, 2016

Uh oh!

jmdavis commented Dec 6, 2016

Uh oh!

9il commented Dec 7, 2016

Uh oh!

9il commented Dec 7, 2016

Uh oh!

9il commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

9il commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

9il commented Dec 7, 2016

Uh oh!

9il commented Dec 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edi33416 commented Dec 9, 2016

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

9il commented Dec 7, 2016 •

edited

Loading

9il commented Dec 7, 2016 •

edited

Loading