Fix Issue 23304 - Add core.bitops.ReverseBitRange #14380
Conversation
|
Thanks for your pull request and interest in making D better, @keivan-shah! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#14380" |
|
Why do we need a different range for this? Can't we just upgrade BitRange to be a bidirectional range? |
|
@RazvanN7 Yes even that is an option, would be a bit more difficult to implement so did not attempt. Will try to implement and update the PR. Thanks for the input! |
|
I think this implements the bidirectional range on the BitArray and should be backwards compatible, but I am not sure about the code runtime. The |
The size problem can indeed be solved by creating a ReverseBitRange that contains a BitRange field and the extra bidirectional range fields, implements all the bidirectional range primitives and forwards anything else to the BitRange field via alias this. Something along the lines of: struct BitRangeReverse
{
BitRange br;
private { /* extra fields go here */}
/* bidirectional range primitives go here */
alias br this;
} |
|
@atilaneves do you think adding a BitRangeReverse is worthwhile? |
| foreach (b; bitsToTest) | ||
| { | ||
| assert(!br.empty); | ||
| assert(b == br.front); | ||
| br.popFront(); | ||
| } | ||
| assert(br.empty); | ||
| foreach_reverse (b; bitsToTest) |
There was a problem hiding this comment.
This should be a separate unittest.
| // reverse iterate | ||
| testSum = 0; | ||
| nBits = 0; | ||
| foreach_reverse (b; BitRange(bitArr, 100)) |
There was a problem hiding this comment.
This should be a separate unittest.
|
What about an adapter that takes a BitRange and returns a BidirectionalBitRange? |
|
So my usecase for the reverse BitRange was to have an optimized range over the bitset in reverse. Now I am not sure if the bidirectional range is the way to go for it since it has both space-time penalty over only a ReverseBitRange. I can check in the |
Right. It was the extra data members that got me to ask the question about an adapter. |
|
Technically, the druntime library usually contains things that the compiler specifically knows about, not necessarily user-facing common used functions. So I would argue that a ReverseBitRange should most likely be in the standard library (phobos), not in the runtime one. BitRange exists because it is used internally in druntime However, we already have BitArray. It does not provide range primitives but can easily be used to create a range of its reverse. So I would argue that ReverseBitRange should not be included in druntime. |
|
Makes sense. In that case should I just close this PR and mark the issue as resolved with a comment mentioning not added since would not be a good fit for the druntime? |
|
Yes, I will take care of this. Thank you for your time and I hope you don't see this as a wasted effort. |
|
No worries @RazvanN7, It was fun trying to contribute, Thanks for your efforts as well! In case anyone comes across this PR looking for only the struct ReverseBitRange
{
/// Number of bits in each size_t
enum bitsPerWord = size_t.sizeof * 8;
private
{
const(size_t)*bits; // points at next word of bits to check
size_t cur; // needed to allow searching bits using bsf
size_t idx; // index of current set bit
size_t len; // number of bits in the bit set.
}
@nogc nothrow pure:
/**
* Construct a BitRange.
*
* Params:
* bitarr = The array of bits to iterate over
* numBits = The total number of valid bits in the given bit array
*/
this(const(size_t)* bitarr, size_t numBits) @system
{
bits = bitarr + (numBits - 1) / bitsPerWord;
len = numBits;
idx = numBits - 1;
if (len)
{
// prime the first bit
cur = *bits-- ^ (size_t(1) << idx);
popFront();
}
}
/// Range functions
size_t front()
{
assert(!empty);
return idx;
}
/// ditto
bool empty() const
{
return idx < 0;
}
/// ditto
void popFront() @system
{
// clear the current bit
auto curbit = idx % bitsPerWord;
cur ^= size_t(1) << curbit;
if (!cur)
{
// find next size_t with set bit
idx -= curbit;
while (!cur)
{
if ((idx -= bitsPerWord) >= len) // WARN: Relying on size_t overflow!
// now empty
return;
cur = *bits--;
}
idx += bsr(cur);
}
else
{
idx -= curbit - bsr(cur);
}
}
} |
No description provided.