aherbert commented on code in PR #329:
URL:
https://github.com/apache/commons-collections/pull/329#discussion_r941544205
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -29,6 +29,15 @@
*/
public interface BloomFilter extends IndexProducer, BitMapProducer {
+ /**
+ * The sparse characteristic used to determine the best method for
matching.
+ * <p>For `sparse` implementations
+ * the {@code forEachIndex(IntConsumer consumer)} method is more
efficient. For non `sparse` implementations
+ * the {@code forEachBitMap(LongConsumer consumer)} is more efficient.
Implementers should determine if it is easier
+ * for the implementation to produce indexes of bit map blocks.</p>
+ */
+ int SPARSE=0x1;
Review Comment:
`SPARSE = 0x1`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -38,17 +47,12 @@ public interface BloomFilter extends IndexProducer,
BitMapProducer {
// Query Operations
/**
- * This method is used to determine the best method for matching.
- *
- * <p>For `sparse` implementations
- * the {@code forEachIndex(IntConsumer consumer)} method is more
efficient. For non `sparse` implementations
- * the {@code forEachBitMap(LongConsumer consumer)} is more efficient.
Implementers should determine if it is easier
- * for the implementation to produce indexes of bit map blocks.</p>
- *
- * @return {@code true} if the implementation is sparse {@code false}
otherwise.
- * @see BitMap
+ * Returns the bitmap of characteristics of the filter.
Review Comment:
Drop the `bitmap of`. I think it confusing as `bitmap` in this package is
specifically used to refer to a range of indices [0, 63] packed into a long.
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -38,17 +47,12 @@ public interface BloomFilter extends IndexProducer,
BitMapProducer {
// Query Operations
/**
- * This method is used to determine the best method for matching.
- *
- * <p>For `sparse` implementations
- * the {@code forEachIndex(IntConsumer consumer)} method is more
efficient. For non `sparse` implementations
- * the {@code forEachBitMap(LongConsumer consumer)} is more efficient.
Implementers should determine if it is easier
- * for the implementation to produce indexes of bit map blocks.</p>
- *
- * @return {@code true} if the implementation is sparse {@code false}
otherwise.
- * @see BitMap
+ * Returns the bitmap of characteristics of the filter.
+ * <p>
+ * Characteristics are defined as bits witin the characteristics integer.
Review Comment:
`within`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilter.java:
##########
@@ -194,7 +194,7 @@ public boolean merge(Hasher hasher) {
@Override
public boolean merge(BloomFilter other) {
Objects.requireNonNull(other, "other");
- if (other.isSparse()) {
+ if ((other.characteristics()&SPARSE)>0) {
Review Comment:
whitespace: `(other.characteristics() & SPARSE) != 0`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/SparseBloomFilter.java:
##########
@@ -58,7 +58,7 @@ public SparseBloomFilter(BloomFilter other) {
Objects.requireNonNull(other, "other");
this.shape = other.getShape();
this.indices = new TreeSet<>();
- if (other.isSparse()) {
+ if ((other.characteristics()&SPARSE)>0) {
Review Comment:
whitespace: `(other.characteristics() & SPARSE) != 0`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/SparseBloomFilter.java:
##########
@@ -169,7 +169,7 @@ public boolean merge(Hasher hasher) {
@Override
public boolean merge(BloomFilter other) {
Objects.requireNonNull(other, "other");
- IndexProducer producer = other.isSparse() ? (IndexProducer) other :
IndexProducer.fromBitMapProducer(other);
+ IndexProducer producer = (other.characteristics()&SPARSE)>0 ?
(IndexProducer) other : IndexProducer.fromBitMapProducer(other);
Review Comment:
whitespace: `(other.characteristics() & SPARSE) != 0`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilter.java:
##########
@@ -65,7 +65,7 @@ public SimpleBloomFilter(BloomFilter other) {
this.shape = other.getShape();
this.bitMap = new
long[BitMap.numberOfBitMaps(shape.getNumberOfBits())];
this.cardinality = 0;
- if (other.isSparse()) {
+ if ((other.characteristics()&SPARSE)>0) {
Review Comment:
whitespace: `(other.characteristics() & SPARSE) != 0`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -69,7 +73,7 @@ public interface BloomFilter extends IndexProducer,
BitMapProducer {
*/
default boolean contains(BloomFilter other) {
Objects.requireNonNull(other, "other");
- return isSparse() ? contains((IndexProducer) other) :
contains((BitMapProducer) other);
+ return (characteristics()&SPARSE)>0 ? contains((IndexProducer) other)
: contains((BitMapProducer) other);
Review Comment:
whitespace: `(characteristics() & SPARSE) != 0`
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -144,7 +148,7 @@ default boolean merge(Hasher hasher) {
Objects.requireNonNull(hasher, "hasher");
Shape shape = getShape();
// create the bloomfilter that is most likely to merge quickly with
this one
- BloomFilter result = isSparse() ? new SparseBloomFilter(shape, hasher)
: new SimpleBloomFilter(shape, hasher);
+ BloomFilter result = (characteristics()&SPARSE)>0 ? new
SparseBloomFilter(shape, hasher) : new SimpleBloomFilter(shape, hasher);
Review Comment:
whitespace: `(characteristics() & SPARSE) != 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: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
