You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2025/12/15 17:14:51 UTC
[PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
dcapwell opened a new pull request, #264:
URL: https://github.com/apache/cassandra-accord/pull/264
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2632644324
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
+ return;
+
+ bits |= (-1L >>> (64 - (toExclusive & 63))) & (-1L << (fromInclusive & 63));
}
+ @Override
public boolean get(int i)
{
+ if (i >= 64) return false;
Review Comment:
we spoke in slack, switched to IIOBE. if `i == size()` then we accept and return `-1` for `nextSetBit` but `get` now rejects.
If `i > size()` we will reject for all input
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2620242634
##########
accord-core/src/main/java/accord/utils/LargeBitSet.java:
##########
@@ -174,9 +179,12 @@ public boolean unset(int i)
return true;
}
+ @Override
public final boolean get(int i)
{
int index = indexOf(i);
+ if (index == UNKNOWN)
Review Comment:
all read APIs default to the negative case if you ask out of bounds. This keeps things consistent and easy to work with.
The write APIs all reject as its clear you did something wrong.
One thing we talked about in slack was exposing capacity, but that also implies that every caller gets updated to check that in the loop where as the pattern has been
```
for (int index = set.nextSetBit(0); index != -1; index = set.nextSetBit(index))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2620248016
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,51 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
+ return;
+
+ long maskTo = (toExclusive >= 64) ? -1L : ~(-1L << toExclusive);
+ bits |= maskTo & (-1L << fromInclusive);
Review Comment:
in slack benedict recommend we use the logic from large: `|= (-1L >>> (64 - (to & 63))) & (-1L << (from & 63))`
Need to test this out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell merged PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2632667487
##########
accord-core/src/main/java/accord/utils/LargeBitSet.java:
##########
@@ -443,10 +448,16 @@ private int indexOf(int i)
{
int index = i >>> 6;
if (index >= length)
- throw new IndexOutOfBoundsException(String.format("%d >= %d", index, length));
+ throw new IndexOutOfBoundsException(String.format("Unable to access bit %d; must be between 0 and %d", i, size() - 1));
Review Comment:
this keeps the 2 impl consistent even with the error type + msg
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2632667487
##########
accord-core/src/main/java/accord/utils/LargeBitSet.java:
##########
@@ -443,10 +448,16 @@ private int indexOf(int i)
{
int index = i >>> 6;
if (index >= length)
- throw new IndexOutOfBoundsException(String.format("%d >= %d", index, length));
+ throw new IndexOutOfBoundsException(String.format("Unable to access bit %d; must be between 0 and %d", i, size() - 1));
Review Comment:
this keeps the 2 impl consistent even with the error type + msg. I also find `index >= length` harder to work with than the actual input / size
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#issuecomment-3720344237
got +1 from Benedict in slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2623709079
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
+ return;
+
+ bits |= (-1L >>> (64 - (toExclusive & 63))) & (-1L << (fromInclusive & 63));
}
+ @Override
public boolean get(int i)
{
+ if (i >= 64) return false;
Review Comment:
shouldn't this be IIOBE?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2623706219
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
Review Comment:
this shouldn't be needed I don't *think*, as the below should simply become 0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2632692763
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
Review Comment:
not safe it seems
```
40: Clear
41: setRange(0, 0)
```
```
Expected :0
Actual :64
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2623720171
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
Review Comment:
`i >>> 6 > 0` or `(i & ~63) != 0`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2624110580
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
Review Comment:
i mostly kept it as the logic was in large, i can remove and confirm with the tests (tests don't hit this case, so will update)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2632664324
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
Review Comment:
went with `i >>> 6 > 0` to be consistent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2632692763
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
Review Comment:
not safe it seems; need to keep this logic
```
40: Clear
41: setRange(0, 0)
```
```
Expected :0
Actual :64
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2620246845
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,51 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
+ return;
Review Comment:
making this consistent with large; which does this same work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2620253614
##########
accord-core/src/test/java/accord/utils/SimpleBitSetTest.java:
##########
@@ -167,9 +234,27 @@ public int nextSetBit(int fromIndex)
@Override
public int getSetBitCount()
{
+ Assertions.assertThat(sut.getSetBitCount())
+ .describedAs(toBinaryString())
+ .isEqualTo(model.cardinality());
return sut.getSetBitCount();
}
+ public String toBinaryString()
+ {
+ StringBuilder sb = new StringBuilder();
+ for (int i = 0; i < size; i++)
+ {
+ boolean m = model.get(i);
+ boolean s = sut.get(i);
+ if (m == s)
+ sb.append(m ? 1 : 0);
+ else
+ sb.append("(expected=").append(m ? 1 : 0).append(", actual=").append(s ? 1 : 0).append(')');
Review Comment:
this format was hacked together to aid in debugging `setRange` issue. Was simple and showed exactly what was needed, but open to other formats.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2620245313
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,51 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
Review Comment:
`to` was exclusive but wouldn't work if you gave the last index as it would cause the bits to be `0`; found in fuzz testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] bitset impl are not consistent, so updated so both implementations are in-sync and to fix tests in Cassandra [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #264:
URL: https://github.com/apache/cassandra-accord/pull/264#discussion_r2624128289
##########
accord-core/src/main/java/accord/utils/SmallBitSet.java:
##########
@@ -40,29 +40,50 @@ public long bits()
return bits;
}
+ private static long bitSafe(int i)
+ {
+ validateIndex(i);
+ return bit(i);
+ }
+
+ private static void validateIndex(int i)
+ {
+ if (i >= 64 || i < 0)
+ throw new IndexOutOfBoundsException("Unable to access bit " + i + "; must be between 0 and 63");
+ }
+
+ @Override
public boolean set(int i)
{
- long bit = bit(i);
+ long bit = bitSafe(i);
boolean result = 0 == (bits & bit);
bits |= bit;
return result;
}
@Override
- public void setRange(int from, int to)
+ public void setRange(int fromInclusive, int toExclusive)
{
- bits |= ~(-1L << to) & (-1L << from);
+ validateIndex(fromInclusive);
+ Invariants.requireArgument(fromInclusive <= toExclusive, "from > to (%s > %s)", fromInclusive, toExclusive);
+ if (fromInclusive == toExclusive)
+ return;
+
+ bits |= (-1L >>> (64 - (toExclusive & 63))) & (-1L << (fromInclusive & 63));
}
+ @Override
public boolean get(int i)
{
+ if (i >= 64) return false;
Review Comment:
my argument is that read methods shouldn't IIOBE and should return the negative case as it simplifies the calling logic (would have to update caller to find the capacity and always make sure to avoid crossing it).
`get` isn't the strongest argument for that, but `nextSetBit` i feel is. Having every loop check if `nextSetBit` returned the last element pushes this to every caller and adds clutter all over.
`accord.utils.SortedArrays#fromSimpleBitSet`
```
for (int i = bitSet.firstSetBit(); i >= 0 ; i = bitSet.nextSetBit(i + 1))
```
`accord.coordinate.AbstractCoordination#inflight`
```
for (int i = expectingReply.nextSetBit(0) ; i >= 0 ; i = expectingReply.nextSetBit(i + 1))
```
All these examples are unsafe if the read apis throws IIOBE and really expect out of bounds to return negative case; this is the common assumption throughout the current usage.
We spoke before in my previous patch and you were not a fan of the `get` method having different semantic from `nextSetBit`, so returning the negative case keeps the read logic simple/safe for callers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org