Skip to content

Conversation

@joprodrigues
Copy link
Contributor

Create boolean (or, and, xor and sub) and equality operators, that operate
on two distinct bitsets with different starting points, but same length, in place.

@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

Merging #53 (697c159) into develop (d4be7a4) will increase coverage by 0.80%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #53      +/-   ##
===========================================
+ Coverage    95.80%   96.61%   +0.80%     
===========================================
  Files            5        5              
  Lines          668      826     +158     
===========================================
+ Hits           640      798     +158     
  Misses          28       28              
Impacted Files Coverage Δ
include/boost/dynamic_bitset/dynamic_bitset.hpp 96.40% <100.00%> (+0.91%) ⬆️

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 d4be7a4...697c159. Read the comment docs.

@joprodrigues
Copy link
Contributor Author

My apologies, it's my first time contributing in Github and I've mistakenly added a new commit to this branch (which should be in a different branch), which I have since reverted

@jeking3
Copy link
Contributor

jeking3 commented Apr 17, 2020

So the problem you are trying to solve with this is to allow for operators on subsets of the data; two observations to consider:

  1. Code that can operate on a subset should be able to operate on the entire set, but I believe this change introduces alternate code paths for bounded operations.

  2. Another possible solution to this problem which I believe would be less change to the class would be to consider allowing a bitset to be created from another as a subset (copies data at an offset and length) or view (references data at an offset and a length, with specific guidelines for the caller to manage the lifetime of the original?). Then the existing operators would give you the behavior you want by working with subsets more naturally.

An example of #2, in pseudocode:

set one(...)
set two(...)

Here is how the pull request proposes to do it:

result = one.iand(two, some_offset_into_one, two, some_offset_into_two, length)

Or perhaps instead, think about:

result = one.view(some_offset_into_one, length) & two.view(some_offset_into_two, length)

where view returns a bitset that references the original data at the given offset and length, and then the operator& can be used naturally for an "and" operation.

That said, comments from others that use bitset more regularly would be appreciated.

@joprodrigues
Copy link
Contributor Author

Yes, this change introduces alternate paths. I didn't want to compromise the speed (and correctness!) of the already existing solution.
Regarding your second suggestion, I will think how can I implement it, since it seems more usable.

@joprodrigues
Copy link
Contributor Author

First, I would like to apologise for the time it took me for this change. I've refactored the code, to create the aforementioned "view", which I called span, due to the similarity with C++20 span.

There are some questions I had while doing this work, and I would like to hear your thoughts on them:

  • when the dynamic_bitset is const, I can't create a const dynamic_bitset_span directly. To solve this, I've created a const_dynamic_bitset_span, which is, of course, trivially converted to const dynamic_bitset_span. However, in some cases I have to explicitly create this new intermediate object.
  • the intersects function has a behaviour different than the other pairwise functions: it operates on the subset where both dynamic_bitsets exist, while other functions only accept equal length dynamic_bitsets
  • in the case of non-member binary operators (for instance dynamic_bitset_span(a) + dynamic_bitset_span(b)), what should be return? Right now, it's (a copy of) the complete underlying dynamic_bitset, but return only the subset operated by the dynamic_bitset_span may also make sense.

Thank you for your attention.
(And maybe I should also say, thank you to all contributors for this library!)

@jeking3
Copy link
Contributor

jeking3 commented May 3, 2022

@joprodrigues please rebase on develop for a more complete CI suite

@jeking3
Copy link
Contributor

jeking3 commented Dec 24, 2024

I'd recommend squashing your changes and then rebasing so we can get CI running on this.

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.

2 participants