Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update API #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update API #24

wants to merge 3 commits into from

Conversation

EliSchleifer
Copy link

@EliSchleifer EliSchleifer commented Sep 9, 2021

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.

@EliSchleifer EliSchleifer marked this pull request as ready for review September 9, 2021 23:46
@codecov-commenter
Copy link

Codecov Report

Merging #24 (db9de38) into master (a70ba82) will decrease coverage by 9.42%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
tests/src/bitflags_test.cpp 81.42% <57.14%> (-18.58%) ⬇️
include/bitflags/bitflags.hpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a70ba82...db9de38. Read the comment docs.

@EliSchleifer
Copy link
Author

Thoughts on this?

@m-peko
Copy link
Owner

m-peko commented Sep 17, 2021

Thoughts on this?

I have put comments to the code. Please take a look at those.

@EliSchleifer
Copy link
Author

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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we take a look at the std::bitset implementation, then it's better to call this function reset

@@ -498,25 +498,38 @@ class bitflags : public ImplT {
*
* @return True if all flags are currently set, otherwise false
*/
NODISCARD constexpr bool is_all() const noexcept {
NODISCARD constexpr bool all_set() const noexcept {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with other functions, we should name this function is_all_set

// raw flags (without string representation)
RawFlags raw_flags = RawFlags::all();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@m-peko
Copy link
Owner

m-peko commented Sep 29, 2021

Just realizing..maybe you left comments but they didn't post correctly because I cannot see any comments on the PR

Yeah, I always forget to submit comments. I am not working with Github on a daily basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants