BitArray shift operators by AlexRadch · Pull Request #14240 · dotnet/corefx

AlexRadch · 2016-12-06T18:36:18Z

The BitArray class in System.Collections should include BitWise right and left shift operations. BitArray is used to represent a collection of on\off values and currently contains And, Or, Xor, and Not operations. Adding RightShift and LeftShift would increase the usefulness of the class and provide a canonical, fast solution to save developers the hassle of coming up with their own.
See https://github.com/dotnet/corefx/issues/8719

Unit tests for the above API get from https://github.com/Clockwork-Muse/corefx/commit/4fc128660ad8c61310bbd10d9d767afda8b22910

AlexRadch · 2016-12-06T20:42:30Z

How add new API to BitArray class? What should I change to remove compilation errors on new methods?

error CS1061: 'BitArray' does not contain a definition for 'RightShift' and no extension method 'RightShift' accepting a first argument of type 'BitArray' could be found

RightShift is new method that I added.

danmoseley · 2016-12-06T20:52:48Z

@AlexRadch make sure you update the ref file also (src\System.Collections\ref\System.Collections.cs) as that's what the tests (and any user code) points to.

AlexRadch · 2016-12-06T21:07:50Z

@danmosemsft I updated this file https://github.com/dotnet/corefx/pull/14240/files#diff-9af4fbb6cc264756bf83c6b96c0144b5 but it is not enough.

danmoseley · 2016-12-06T22:02:24Z

@joperezr any idea? I can't see it.

joperezr · 2016-12-07T00:01:12Z

Hi @AlexRadch, first of all thanks for your contribution! You are hitting this issue because we cross compile System.Collections for different frameworks, and one of them is net463 which doesn't use the implementation of BitArray that you modified. This is causing for the error that you called out above, since for net463 build, we don't have these two newly added members. What we need to do in this case, is to condition the reference assembly to only expose these new APIs for .NET Core and UWP, and then make sure that the net463 tests don't include the ones you just wrote, since that API won't be available for that framework.

Do you want to give that a try (with some guidance and instructions from me 😄) or do you want me to take over, and build a commit on top of yours?

joperezr · 2016-12-07T01:07:16Z

FYI: I did go ahead and made some changes that should unblock your PR, feel free to either cherrypick them to your branch or to manually apply them. They are here: joperezr@02de163

AlexRadch · 2016-12-07T03:17:35Z

@joperezr I merge your changes. Thank you!

ianhays

FYI to people who commented on the original issue: @Clockwork-Muse @chrisaut @ryantrem

ianhays · 2016-12-07T16:27:36Z

+        }
+
+        //[Fact]
+        //public static void Shift_Invalid()


Why is this one commented out?

Because I made other direction shift.

Please remove then we try not to have commented out code

ianhays · 2016-12-07T16:37:43Z

+                }
+                count = -count;
+                // Following trick reduce check with int.MinValue
+                if (unchecked((uint)count < (uint)m_length))


Seems needlessly complicated. Why not just have:

if (count == 0) return this; if (count < 0) return LeftShift(-count);

And let LeftShift do the validation to ensure -count is a valid shift.

if (count < 0) return LeftShift(-count);

Stack overflow with count = int.MinValue -int.MinValue == int.MinValue)
Also one check is better that two. It is trick code.

ianhays · 2016-12-07T16:39:44Z

+            {
+                if (count == 0)
+                {
+                    _version++;


Still be updating the version if no modifications are made?

Yes, it is usual behavior of all BitArray methods. See Not method when len = 0 for example.

AlexRadch · 2016-12-09T02:45:16Z

@Clockwork-Muse @chrisaut @ryantrem CR please

chrisaut · 2016-12-09T03:30:52Z

+            if (unchecked((uint)count < (uint)m_length))
+            {
+                var fromIndex = count / BitsPerInt32;
+                int shiftCount = count % BitsPerInt32;


Perhaps you could use Math.DivRem here? Good discussion about jit behaviour here https://github.com/dotnet/coreclr/issues/3439

I do not know why but I got compilation error on Math.DivRem: System\Collections\BitArray.cs(432,38): error CS0117: 'Math' does not contain a definition for 'DivRem'

@AlexRadch I guess that method is not exposed in whatever targetframework the repo is targeting. Either that or the dependency chain is such that BitArray is lower in the dependency chain than the Math implementation. I seriously doubt that though.

I see you've inlined the implementation. I'm not sure that's a good idea as the jitter should eventually recognized div followed by rem and use the results from the hardware immediately (which is why the change to Math.DivRem is temporary https://github.com/stephentoub/coreclr/blob/64edfc9b668d2f4af8c32d7dda2e64e776b40af1/src/mscorlib/src/System/Math.cs#L729).

In any case, I don't think its a big deal either way. Sorry for the confusion.

chrisaut · 2016-12-09T03:31:19Z

+            if (unchecked((uint)count < (uint)m_length))
+            {
+                lastIndex = (m_length - 1) / BitsPerInt32;
+                int shiftCount = count % BitsPerInt32;


Same comment about Math.DivRem

chrisaut · 2016-12-09T03:31:36Z

Sorry I cannot review in detail. But I am happy this is going in, and from a quick glance it looks like a sensible implementation. Good job and thnx for pinging me.

Clockwork-Muse · 2016-12-09T16:13:33Z

Generally speaking, you shouldn't be pulling from master into a PR, since it confuses what's new. Unless you need something from a recent PR, or there's a merge conflict, just let it get "out of date". If you do need something from master, you should be rebasing instead.
Could you revert the pulls, please?

AlexRadch · 2016-12-09T16:31:29Z

@Clockwork-Muse I made something strange. I try to make rebase. Now I am thinking how to revert all....

joperezr · 2016-12-09T22:42:36Z

I believe what you want to do is to return to the original state of your PR and then rebase with master. You can try running this commands in order to fix your problem and update this PR accordingly:
(These next instructions are assuming that your remote for dotnet/corefx is called 'upstream', and AlexRadch/corefx is 'origin')

git checkout BitArray-Shift
git reset --hard 3a2db757adeae49e18148a267723d4a42139ce4a
git fetch upstream master
git rebase upstream/master
(Previous command will give you all of the conflicts, if any. Fix them by calling 'git status' to see which ones you are missing. Once you are done merging conflicts, you should be able to call 'git rebase --continue'. Repeat this process as many times as necessary since it will run 7 times and pause each time you have a conflict)
git push -f origin BitArray-Shift

That should get you into a good state again. Let me know if you run into any issues.

danmoseley · 2016-12-10T00:29:38Z

@AlexRadch not sure if it helps, but I have found a UI (I use SourceTree) helpful to visualize the state of branches when I do something like this. For example after the reset it shows just where your branches are.

AlexRadch · 2016-12-10T03:25:40Z

I made correct rebase, I think. @ianhays @danmosemsft @chrisaut code review please

danmoseley · 2016-12-11T02:16:57Z

The rebase is good @ianhays should sign off on the code/tests

ryantrem · 2016-12-12T03:30:29Z

Glad to see this getting some traction!

ryantrem · 2016-12-12T03:34:23Z

+        ** Shift all the bit values to right on count bits. When count less zero it
+        ** shift all the bit values to left. New bits are be set to zero. The 
+        ** current instance is updated and returned.
+        =========================================================================*/


It seems weird and a bit confusing to me that you can either invoke LeftShift(n) or RightShift(-n) and get the same behavior. I'd think either negative values would not be valid, or there would be a single Shift method that accepts negative or positive values. Ultimately though, to me it makes the most sense to just match the behavior of the primitive shift operators (which this currently does not).

Shift operations on int and uint just give five bits of the count. In fact they make next operations x shift (count & 0b11111).

BitArray have any length so we can not give some bits of count.

Left shift is like multiplication by 2^n. (x << count) == (x * 2 ^ count)
Right shift is like division by 2^n. (x >> count) == (x / 2 ^ count)
Multiplication and Division are defined for count less than 0 like next:
(x * 2 ^ count) == (x / 2 ^ -count)
So it have a mean to shift in other direction.

Primitive shift operators does not throw exceptions. So if count < 0 we must:

Throw argument out of range exception.

Shift on zero bits.

Shift on -count bits in other direction.

What is you choice?

@AlexRadch

int x = 8; Console.WriteLine(x << 1); Console.WriteLine(x << -1); Console.WriteLine(x << 0); Console.WriteLine(x >> 1); Console.WriteLine(x >> -1); Console.WriteLine(x >> 0);

gives output,

16 0 8 4 0 8

The c# shift operators just return 0 on negative count. But on BitArray, making it 0 would not be a desired behavior in manipulating the array in place to an unexpected value. Same case for changing the shift direction as well, when the API explicitly mentions right shift, and the array being manipulated in a different shift. I would prefer if an exception is thrown.

@Priya91 we can not use int shift logic try next in interactive

> int x = 1; > x << -1 -2147483648

I will throw exception

ryantrem · 2016-12-12T03:35:37Z

+                }
+                count = -count;
+                // Following trick reduce check with int.MinValue
+                if (unchecked((uint)count < (uint)m_length))


Why all the explicit usage of unchecked when this is already the default for non-constants?

count = -count have one special case. If count equal to int.MinValue than it stay int.MinValue (less zero). So to check this special case in one operation I used such trick code.

joperezr · 2016-12-12T17:56:48Z

I made correct rebase, I think. @ianhays @danmosemsft @chrisaut code review please

@AlexRadch I don't think that the rebase si right, I still see some of your commits twice. Can you try running the commands I pointed out above?

AlexRadch · 2016-12-12T19:44:42Z

@joperezr Git have me again

AlexRadch · 2016-12-12T20:23:15Z

@joperezr I do not know what I do but I created only one commit.

AlexRadch · 2016-12-13T04:47:11Z

@ianhays, @ryantrem, @danmosemsft, @chrisaut I resolved merge conflicts. Review code please.

joperezr · 2016-12-13T16:49:17Z

I do not know what I do but I created only one commit.

you squashed everything in one commit, which for PRs is ideally better to keep in multiple commits so that we can see the different iterations of the changes, but squashing all to one is not the end of the world 😄

ianhays · 2016-12-14T17:55:45Z

+
+                _version++;
+                return this;
+            }


nit: no need to nest the ifs like this

if (count == 0) { version++; return this; } if (count < 0) { throw new ArgumentOutOfRangeException(nameof(count), count, SR.ArgumentOutOfRange_NeedNonNegNum); }

if (count <= 0) { if (count < 0) { } }

Made 1 comparison in usual flow and 2 in rare flow.

if (count == 0) { } if (count < 0) { }

Made 2 comparison in usual flow.

You're right, I just prefer the other style as it's a bit easier to read and follows our style throughout the file better. You can leave it like this if you think it's better.

I like easy to read but this is system library so I use such performance tricks.

ianhays · 2016-12-14T17:56:13Z

+            if (count < m_length)
+            {
+                int shiftCount;
+                // int fromIndex = Math.DivRem(count, BitsPerInt32, out shiftCount); // Compilation error


Why is this comment here?

Here was review to use Math.DivRem but it can not be compiled. So I inserted this comment.

We try to avoid commented out code in source. Better to either make it work, remove it, or add an actual comment explaining in detail why we're not using Math.

ianhays · 2016-12-14T17:57:41Z

+                int lastIndex = (m_length - 1) / BitsPerInt32;
+
+                int shiftCount;
+                //lengthToClear = Math.DivRem(count, BitsPerInt32, out shiftCount); // Compilation error


ditto on comment

ianhays · 2016-12-15T19:11:50Z

+            if (count < m_length)
+            {
+                int shiftCount;
+                // We can not use Math.DivRem due compilation error


nit: // We can not use Math.DivRem without taking a dependency on System.Runtime.Extensions

ianhays · 2016-12-15T19:14:12Z

+                {
+                    throw new ArgumentOutOfRangeException(nameof(count), count, SR.ArgumentOutOfRange_NeedNonNegNum);
+                }
+                Contract.EndContractBlock();


FYI Contract.EndContractBlock(); isn't necessary anymore, so you can leave it out in future additions. It's used in every other method in this file so I see why you added it though.

@ianhays ok

ianhays

That's all my feedback! @Priya91 mind taking a look?

…untime.Extensions

AlexRadch · 2016-12-19T14:50:43Z

@ianhays @ryantrem @danmosemsft @Priya91 @chrisaut Code review please

ianhays

LGTM. I'd like another pair of eyes to take a quick look before merging if possible. If no one bites then it should still be good-to-go though.

danmoseley · 2016-12-27T17:31:22Z

@dotnet-bot test Innerloop OSX Release Build and Test please (log gone)
@mmitche idea -- could we keep failing logs longer? Eg., for 1-2 weeks, while passing logs still get deleted after 1-2 days? Most logs are passing, and we don't care about them. Quite often I go to someone else's PR (particularly from external contributor who is not going to necessarily look immediately) to figure out a failure and the log has gone, which means a rerun which means delay and unnecessary load on the CI system.

danmoseley · 2016-12-27T17:32:54Z

@Priya91 @chrisaut do you want to CR? If not @Priya91 or @ianhays I think you can hit merge when yo'ure back from the holidays.

mmitche · 2017-01-03T16:22:44Z

@danmosemsft Yeah I think so. I may alter the policy again to keep Pr builds longer than official builds.

BitArray shift operators Commit migrated from dotnet/corefx@851f91e

dnfclas added the cla-already-signed label Dec 6, 2016

karelz assigned AlexRadch, ianhays and Priya91 Dec 7, 2016

karelz added the area-System.Collections label Dec 7, 2016

ianhays reviewed Dec 7, 2016

View reviewed changes

chrisaut reviewed Dec 9, 2016

View reviewed changes

ryantrem reviewed Dec 12, 2016

View reviewed changes

AlexRadch closed this Dec 12, 2016

BitArray Shift

e67ede7

AlexRadch reopened this Dec 12, 2016

dnfclas added the cla-already-signed label Dec 12, 2016

Exceptions: ArgumentOutOfRangeException if count < 0

4480f57

ianhays reviewed Dec 14, 2016

View reviewed changes

Math.DivRem comment

f586354

ianhays reviewed Dec 15, 2016

View reviewed changes

// We can not use Math.DivRem without taking a dependency on System.R…

acc0aeb

…untime.Extensions

ianhays approved these changes Dec 19, 2016

View reviewed changes

ryantrem approved these changes Dec 26, 2016

View reviewed changes

ianhays merged commit 851f91e into dotnet:master Dec 27, 2016

karelz modified the milestone: 2.0.0 Dec 27, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#14240 from AlexRadch/BitArray-Shift

e467217

BitArray shift operators Commit migrated from dotnet/corefx@851f91e

Conversation

AlexRadch commented Dec 6, 2016

Uh oh!

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

Uh oh!

Uh oh!

danmoseley commented Dec 6, 2016

Uh oh!

AlexRadch commented Dec 6, 2016

Uh oh!

danmoseley commented Dec 6, 2016

Uh oh!

joperezr commented Dec 7, 2016

Uh oh!

joperezr commented Dec 7, 2016

Uh oh!

AlexRadch commented Dec 7, 2016

Uh oh!

ianhays 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!

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

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!

AlexRadch commented Dec 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisaut Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisaut commented Dec 9, 2016

Uh oh!

Clockwork-Muse commented Dec 9, 2016

Uh oh!

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

Uh oh!

Uh oh!

joperezr commented Dec 9, 2016

Uh oh!

danmoseley commented Dec 10, 2016

Uh oh!

AlexRadch commented Dec 10, 2016

Uh oh!

danmoseley commented Dec 11, 2016

Uh oh!

ryantrem commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexRadch Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AlexRadch commented Dec 6, 2016 •

edited

Loading

AlexRadch Dec 7, 2016 •

edited

Loading

chrisaut Dec 11, 2016 •

edited

Loading

AlexRadch commented Dec 9, 2016 •

edited

Loading

AlexRadch Dec 12, 2016 •

edited

Loading

AlexRadch Dec 14, 2016 •

edited

Loading