Conversation
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
===========================================
- Coverage 100.00% 90.57% -9.43%
===========================================
Files 2 2
Lines 122 138 +16
===========================================
+ Hits 122 125 +3
- Misses 0 13 +13
Continue to review full report at Codecov.
|
|
Thoughts on this? |
I have put comments to the code. Please take a look at those. |
|
Just realizing..maybe you left comments but they didn't post correctly because I cannot see any comments on the PR |
| * @return True if only the specified flag is in the | ||
| * current set of flags, otherwise false | ||
| */ | ||
| NODISCARD constexpr bool only_set(FlagT<ImplT, T> const& rhs) const noexcept { |
There was a problem hiding this comment.
For this, you have is_set / contains function
| * | ||
| * @param rhs Flag to be unset | ||
| */ | ||
| NON_CONST_CONSTEXPR void clear(FlagT<ImplT, T> const& rhs) noexcept { |
There was a problem hiding this comment.
If we take a look at the std::bitset implementation, then it's better to call this function reset
| * @return True if all flags are currently set, otherwise false | ||
| */ | ||
| NODISCARD constexpr bool is_all() const noexcept { | ||
| NODISCARD constexpr bool all_set() const noexcept { |
There was a problem hiding this comment.
To be consistent with other functions, we should name this function is_all_set
| TEST_F(BitflagsTest, All) { | ||
| TEST_F(BitflagsTest, DISABLED_All) { | ||
| // raw flags (without string representation) | ||
| RawFlags raw_flags = RawFlags::all(); |
There was a problem hiding this comment.
all method is supposed to return bitflags with all bits set to true. However, this might not be the perfect naming. We should consider renaming is_all_set -> all (this will check if all the bits are set to true) and all -> set (without parameter) - this will set all the bits to true.
+
we can add all/any/none like here
Yeah, I always forget to submit comments. I am not working with Github on a daily basis. |
3 participants
Renames
contains > is_set
remove(Flag) > clear(Flag)
is_all > all_set
I think I found a bug in the All code. The All() method I believe is returning all bits, not just defined flags. I updated the test to show this problem. Wasn't sure exactly how to fix it and figured you'd have thoughts.
I disabled the broken test.
Also didn't update documentation. DIdn't want to churn until you were good with the API changes.