Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement <<= and >>= for std.bitmanip.BitArray. #2797

Merged
merged 2 commits into from
Jan 4, 2015

Conversation

quickfur
Copy link
Member

ptr[i] = 0;
}

version(none)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bad idea to introduce dead code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops!! I was supposed to delete that code before pushing. Sorry!!

@schuetzm
Copy link
Contributor

>> is a signed right-shift, >>> is for unsight right-shift, see here:
http://dlang.org/expression#ShiftExpression
I think BitArray should behave the same for consistency.

@yebblies
Copy link
Member

>> is a signed right-shift, >>> is for unsight right-shift, see here:

>> is identical to >>> for unsigned integral types. What exactly are you suggesting?

@schuetzm
Copy link
Contributor

Is it? The documentation doesn't say so. Anyway, then it's ok to leave it as-is.

@yebblies
Copy link
Member

Is it? The documentation doesn't say so.

Yeah, that documentation could be clearer. IIRC >> is defined in C as a division by a power of two.

@quickfur
Copy link
Member Author

Rebased. Are we talking about dlang.org docs, or is there some docs in this PR that needs to be improved?

@yebblies
Copy link
Member

Are we talking about dlang.org docs, or is there some docs in this PR that needs to be improved?

dlang.org

@@ -1710,6 +1710,242 @@ public:
assert(c[2] == 0);
}

// Rolls double word (upper, lower) to the right by n bits and returns the
// lower word of the result.
private static size_t rollRight(size_t upper, size_t lower, size_t nbits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really-really hope this is inlined but experience tells me it's likely not.
I'd double check it - if not, then using empty template args rollRight()(sizet_ ...) would help.

@quickfur
Copy link
Member Author

quickfur commented Jan 4, 2015

Templatized rollLeft and rollRight. Anything else?

@ghost
Copy link

ghost commented Jan 4, 2015

LGTM. Anyone else?

@DmitryOlshansky
Copy link
Member

LGTM

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

DmitryOlshansky added a commit that referenced this pull request Jan 4, 2015
Implement <<= and >>= for std.bitmanip.BitArray.
@DmitryOlshansky DmitryOlshansky merged commit bc6ba4c into dlang:master Jan 4, 2015
@quickfur quickfur deleted the bitarray_shift branch January 4, 2015 22:54
@ghost
Copy link

ghost commented Jan 4, 2015

Nice! So, are we going to implement rotations too?

@quickfur
Copy link
Member Author

quickfur commented Jan 4, 2015

Should we?

@MartinNowak
Copy link
Member

Should we?

Sounds useful.

// Unfortunately, due to the way core.bitop.bt() works, indices are the
// reverse order of what an equivalent << on a size_t would do, so we
// have to use rollRight() instead of rollLeft() here, contrary to the
// direction of the << operator on this BitArray.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this comment, but bt sure works like I thought it does.
http://dpaste.dzfl.pl/005ca912e356

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what's going on, you're shifting in the wrong directions.
In fact the formatted string is LSB order, so shifting in one direction should shift the string into the other.

import std.bitmanip, std.format;

void main()
{
    BitArray ba;
    ba.init([0, 0, 0, 0, 1]);
    assert(ba[4]);
    assert(format("%b", ba) == "00001");
    ba >>= 2;
    assert(ba[2]);
    assert(format("%b", ba) == "001");
    ba <<= 2;
    assert(ba[4]);
    assert(format("%b", ba) == "00001");
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the directions of the shift should be reversed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@quickfur
Copy link
Member Author

quickfur commented Jan 5, 2015

Follow-up pull: #2844

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants