[FEATURE] dynamic_bitset #1153

eseiler · 2019-07-03T19:16:27Z

Immediate TODO:

resize needs to set bits to 0 if shrinking
Check side effects of above change
rough docs
refine docs
snippets
Take a guess which functions are not called, because codecov isn't marking them
increase coverage
replace ctor + resize with string ctor in tests
to_string() - with custom debug_stream overload if this does not work automatically when to_string() is defined. Standard range debug_stream'ing needs to be reversed.

Maybe TODO:

Lavish features such as:

std::string to_string(CharT zero = CharT('0'), CharT one = CharT('1')) const; which allows you to set specific characters for 0 and 1
unsigned long to_ulong() const and unsigned long long to_ullong() const, but these are one-liners
std::hash support

Note:

1️⃣ begin() points to the least significant bit, i.e. right to left instead left to right.
Example:

seqan3::dynamic_bitset t1{0b1100'0001};
t1[0]; // 1
t1[1]; // 0
...
t1[6]; // 1
t1[7]; // 1

for (auto it = t1.begin(); it != it.end(); ++it)
    *it; // 10000011

Future TODO:

extend for arbitrary size
For operator>> we have the line:

  if (is.eof())
      break;

I copied from the small_string implementation.
It's used correctly in this context (we check is.eof() after we read EOF), but we only read up to max(is.width(), arg.max_size()) many characters, which already ensures we never read EOF, so the branch is never reached. @smehringer

include/seqan3/range/container/dynamic_bitset.hpp

marehr · 2019-07-03T19:58:38Z

Just curious, why is 58 the upper limit?

eseiler · 2019-07-03T20:02:51Z

Just curious, why is 58 the upper limit?

We use the most significant 6 bit of the uint64_t to encode the size.

marehr · 2019-07-03T20:24:43Z

A normal bitset can also be greater than 64bit, this one can't. I imagine you don't need this for your use case.

codecov · 2019-07-03T20:25:35Z

Codecov Report

Merging #1153 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1153     +/-   ##
=========================================
+ Coverage   96.72%   96.83%   +0.1%     
=========================================
  Files         210      211      +1     
  Lines        8003     8280    +277     
=========================================
+ Hits         7741     8018    +277     
  Misses        262      262

Impacted Files	Coverage Δ
include/seqan3/range/container/dynamic_bitset.hpp	`100% <100%> (ø)`
...de/seqan3/argument_parser/detail/version_check.hpp	`92.74% <0%> (+0.11%)`	⬆️

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 1527991...7dd31a7. Read the comment docs.

eseiler · 2019-07-03T20:26:56Z

A normal bitset can also be greater than 64bit, this one can't. I imagine you don't need this for your implementation.

For now this is enough, main application will be the shapes, where 32 is fine.
But future plan is to then extend it for > 64

marehr

A general design question:

If you want to encode the size and value into one integer, I think it would be better to use bitfields, because the compiler will do all the hard work for you and it is a standard feature!

https://godbolt.org/z/aNcMKl

include/seqan3/range/container/dynamic_bitset.hpp

eseiler

Change for next push after review

test/snippet/range/container/dynamic_bitset_subscript.cpp

test/snippet/range/container/dynamic_bitset_begin.cpp

marehr

lgtm 👍

include/seqan3/range/container/dynamic_bitset.hpp

eseiler · 2019-07-18T11:17:02Z

@marehr ping

marehr

lgtm

rrahn · 2019-08-14T07:19:39Z

include/seqan3/range/container/dynamic_bitset.hpp

+ * ### Thread safety
+ *
+ * This container provides no thread-safety beyond the promise given also by the STL that all
+ * calls to `const` member function are safe from multiple threads (as long as no thread calls


Suggested change

* calls to `const` member function are safe from multiple threads (as long as no thread calls

* calls to `const` member functions are safe from multiple threads (as long as no thread calls

rrahn · 2019-08-14T08:17:49Z

include/seqan3/range/container/dynamic_bitset.hpp

+        {
+            return !static_cast<bool>(internal.bits & mask);
+        }
+


double newline

rrahn · 2019-08-14T08:21:46Z

include/seqan3/range/container/dynamic_bitset.hpp

+
+    //!\cond
+    // this signals to range-v3 that something is a container :|
+    using allocator_type    = void;


As far as I can tell, the container concept of the ranges library does not check for a type allocator_type. Can you give me an example where the expected behavior does not work without this type?

rrahn · 2019-08-14T08:21:59Z

include/seqan3/range/container/dynamic_bitset.hpp

+
+    /*!\brief Construct from a different range.
+     * \tparam other_range_t The type of range to be inserted; must satisfy std::ranges::InputRange and `value_type`
+     *                       must be constructible from reference_t<other_range_t>.


rrahn · 2019-08-14T08:23:39Z

include/seqan3/range/container/dynamic_bitset.hpp

+    /*!\brief Construct from a different range.
+     * \tparam other_range_t The type of range to be inserted; must satisfy std::ranges::InputRange and `value_type`
+     *                       must be constructible from reference_t<other_range_t>.
+     * \param[in]      range The sequence to construct/assign from.


I mean, leave it as is right now but for the future please do not align different doxygen entities. That looks just awful.

rrahn · 2019-08-14T08:25:37Z

include/seqan3/range/container/dynamic_bitset.hpp

+
+    /*!\brief Assign from pair of iterators.
+     * \tparam begin_it_type Must satisfy std::ForwardIterator and the `value_type` must be constructible from
+     *                       the reference type of begin_it_type.


can you please check all comments again, i.e. that code expressions are wrapped in a code-block. So that it is consistent through the documentation

rrahn · 2019-08-14T08:27:41Z

include/seqan3/range/container/dynamic_bitset.hpp

+     *
+    * \details
+     *
+     * Calling pop_back() on an empty container is undefined. In debug mode an assertion will be thrown.


Suggested change

* Calling pop_back() on an empty container is undefined. In debug mode an assertion will be thrown.

* Calling `pop_back()` on an empty container is undefined. In debug mode an assertion will be thrown.

rrahn · 2019-08-14T08:41:39Z

include/seqan3/range/container/dynamic_bitset.hpp

+    }
+
+    //!\overload
+    constexpr void swap(dynamic_bitset && rhs) noexcept


what are you saving? The constructor and the assignment are defaulted anyway. Non one us using the swap internally. the copy-n-swap idiom is not necessary here. Hence, std::swap would work already for this class and should be the preferred way.
Mhmm but I see know, that it is the stupid container requirement. I'll make this a discussion point in the next dev meeting

include/seqan3/range/container/dynamic_bitset.hpp

rrahn · 2019-08-14T08:43:26Z

include/seqan3/range/container/dynamic_bitset.hpp

+     *
+     * ### Thread-safety
+     *
+     * Not thread-safe.


no you don't need to mark them not-thread safe as it is clear by the top level documentation. In general if there is no explicit statement you always assume not thread-safe

eseiler · 2019-08-14T12:35:39Z

@rrahn all the doc issues should be resolved now

marehr reviewed Jul 3, 2019

View reviewed changes

include/seqan3/range/container/dynamic_bitset.hpp Outdated Show resolved Hide resolved

eseiler force-pushed the feature/constexpr_bitset branch from 6030df9 to 2df2476 Compare July 3, 2019 20:25

eseiler force-pushed the feature/constexpr_bitset branch 4 times, most recently from 5a21b56 to 3b7cd9b Compare July 5, 2019 15:17

eseiler requested a review from marehr July 5, 2019 15:34

eseiler force-pushed the feature/constexpr_bitset branch from 31d8fe9 to 8d0fd8e Compare July 8, 2019 12:58

marehr requested changes Jul 8, 2019

View reviewed changes

eseiler force-pushed the feature/constexpr_bitset branch from 8d0fd8e to 005db2e Compare July 9, 2019 16:38

eseiler requested a review from marehr July 9, 2019 16:39

eseiler force-pushed the feature/constexpr_bitset branch 6 times, most recently from ddd9dec to 048f99e Compare July 11, 2019 13:20

eseiler commented Jul 15, 2019

View reviewed changes

test/snippet/range/container/dynamic_bitset_subscript.cpp Outdated Show resolved Hide resolved

test/snippet/range/container/dynamic_bitset_begin.cpp Outdated Show resolved Hide resolved

marehr reviewed Jul 15, 2019

View reviewed changes

eseiler force-pushed the feature/constexpr_bitset branch from b8f20e0 to 5513d62 Compare July 15, 2019 21:13

eseiler requested a review from marehr July 15, 2019 21:14

marehr approved these changes Jul 18, 2019

View reviewed changes

eseiler requested a review from rrahn July 18, 2019 16:00

rrahn reviewed Aug 14, 2019

View reviewed changes

eseiler added 17 commits August 14, 2019 09:36

[FEATURE] dynamic_bitset

ecc075a

[TEST] Increase code coverage

481edd7

[MISC] Make operator>> not O(n^2)

03543f5

[MISC] Use bit field

5678ce2

[MISC] refactor construction from char literal

9c369eb

[FIX] ICE on gcc < 9

f0e3da9

[MISC] Exclude non-testable line from coverage

5c1221f

[FEATURE] Add debug_stream overload

2669ae4

[FIX] left/right shift

0b14e1e

[DOCS] Snippet, batch 1

d8f9680

[DOCS] Snippets, batch 2

de0f4e1

[TEST] Use char literal ctor where applicable

c78329c

[FEATURE] Add std::hash overload

7930339

[DOCS] Put \attention before example

c86ff70

[FIX] Review 1

2d59605

[FIX] Review 2

54eb964

[MISC] Rename data() to raw_data() and data_ to data

0471903

eseiler force-pushed the feature/constexpr_bitset branch from 1af3b21 to bc0c267 Compare August 14, 2019 07:42

[DOCS] Review 3

9671c33

eseiler force-pushed the feature/constexpr_bitset branch from bc0c267 to 9671c33 Compare August 14, 2019 07:47

eseiler requested a review from rrahn August 14, 2019 07:49

rrahn reviewed Aug 14, 2019

View reviewed changes

[DOCS] Review 4

7dd31a7

eseiler requested a review from rrahn August 14, 2019 12:35

rrahn approved these changes Aug 16, 2019

View reviewed changes

rrahn merged commit 6f2d188 into seqan:master Aug 16, 2019

eseiler deleted the feature/constexpr_bitset branch August 16, 2019 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] dynamic_bitset #1153

[FEATURE] dynamic_bitset #1153

eseiler commented Jul 3, 2019 •

edited

Loading

marehr commented Jul 3, 2019

eseiler commented Jul 3, 2019 •

edited

Loading

marehr commented Jul 3, 2019 •

edited

Loading

codecov bot commented Jul 3, 2019 •

edited

Loading

eseiler commented Jul 3, 2019 •

edited

Loading

marehr left a comment

eseiler left a comment

marehr left a comment

eseiler commented Jul 18, 2019

marehr left a comment

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

rrahn Aug 14, 2019

eseiler commented Aug 14, 2019

	* calls to `const` member function are safe from multiple threads (as long as no thread calls
	* calls to `const` member functions are safe from multiple threads (as long as no thread calls

	* Calling pop_back() on an empty container is undefined. In debug mode an assertion will be thrown.
	* Calling `pop_back()` on an empty container is undefined. In debug mode an assertion will be thrown.

[FEATURE] dynamic_bitset #1153

[FEATURE] dynamic_bitset #1153

Conversation

eseiler commented Jul 3, 2019 • edited Loading

Immediate TODO:

Maybe TODO:

Note:

Future TODO:

marehr commented Jul 3, 2019

eseiler commented Jul 3, 2019 • edited Loading

marehr commented Jul 3, 2019 • edited Loading

codecov bot commented Jul 3, 2019 • edited Loading

Codecov Report

eseiler commented Jul 3, 2019 • edited Loading

marehr left a comment

Choose a reason for hiding this comment

eseiler left a comment

Choose a reason for hiding this comment

marehr left a comment

Choose a reason for hiding this comment

eseiler commented Jul 18, 2019

marehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented Aug 14, 2019

eseiler commented Jul 3, 2019 •

edited

Loading

eseiler commented Jul 3, 2019 •

edited

Loading

marehr commented Jul 3, 2019 •

edited

Loading

codecov bot commented Jul 3, 2019 •

edited

Loading

eseiler commented Jul 3, 2019 •

edited

Loading