Fet/Bitmask #134
Fet/Bitmask #134
Conversation
|
|
||
| using namespace Poly; | ||
|
|
||
| constexpr BitMask::DataType TYPE_BIT = CHAR_BIT * sizeof(BitMask::DataType); |
There was a problem hiding this comment.
constexpr size_t TYPE_BIT = CHAR_BIT * sizeof(BitMask::DataType);
| if (size%TYPE_BIT) | ||
| arraySize = size / TYPE_BIT + 1; | ||
| else | ||
| arraySize = size / TYPE_BIT; |
There was a problem hiding this comment.
Change this whole block to arraySize = (size + TYPE_BIT - 1) / TYPE_BIT;
| BitMask operator|(const BitMask& rhs) const; | ||
| BitMask operator^(const BitMask& rhs) const; | ||
| BitMask operator&(const BitMask& rhs) const; | ||
| BitMask& operator~(); |
There was a problem hiding this comment.
This operator should be const and return a copy, just like &, | and ^.
| bool operator!=(const BitMask rhs) const | ||
| { | ||
| return !(*this == rhs); | ||
| } |
There was a problem hiding this comment.
Put this whole method in one line. This should accept const BitMask& as argument
| { | ||
| return !(*this == rhs); | ||
| } | ||
| bool operator[](size_t index) const; |
There was a problem hiding this comment.
Please add @todo annotation: "Convert this method to return bit proxy", or if you want I can tell you how to write this proxy. In general we want to be able to do sth like this: BitMask b; b[0] = true; which is not possible right now.
|
|
||
| BitMask BitMask::operator^(const BitMask& rhs) const | ||
| { | ||
| if (BitList.GetSize() == rhs.BitList.GetSize()) |
There was a problem hiding this comment.
All three operators (&, |, ^) can be written much simpler. Core algorithm logic looks like this:
- newBitsNumber = max(BitsNumber, rhs.BitsNumber)
- Perform logic operation on values
- Trim returned BitMask to size == position of the leftmost bit set to 1.
We can discuss this further via pm.
| bool Reset(); | ||
| bool Toggle(size_t index); | ||
|
|
||
| bool Resize(const int offset = 0); |
There was a problem hiding this comment.
size_t size not const int offset
| } | ||
|
|
||
|
|
||
| bool BitMask::Resize(const int offset) |
There was a problem hiding this comment.
Resize should apply given size, not current size + offset. That way this method is simple
BitsNumber = size; BitList.Resize((size + TYPE_BIT - 1)/TYPE_BIT)
| return index / TYPE_BIT; | ||
| } | ||
|
|
||
| BitMask& BitMask::operator|=(const BitMask& rhs) |
There was a problem hiding this comment.
No need to write this like that, simply write { *this = *this | rhs; }. Repeat this for other operators overloads.
| bool BitMask::operator==(const BitMask rhs) const | ||
| { | ||
| if (BitsNumber != rhs.BitsNumber) | ||
| return false; |
There was a problem hiding this comment.
Not necessarly, if one of the bitmasks is prefixed with a lot of zeroes this would still be true.
It would probably be helpfull to add method called Trim() which resizes BitMast to size == position of the leftmost bit set to 1. This way you can first trim, or get size after potential trim operation, and compare them to each other.
BitMask working with different unsigned data types. In Unit Tests BitList Dynarray size is checked for Debugging purposes.