Closed HolyBlackCat closed 2 years ago
Wow, thanks for the detailed feedback! All great points I'll address them ASAP
@HolyBlackCat I think I've addressed all of these in my most recent PR #41 (with the default
s being added directly to master). Please re-open if you see anything amiss and again, thank you for reviewing this!
Pardon me for dumping several issues into a single ticket.
bit_vector
lacks a move constructor.digits
should bestatic constexpr
. There's no reason to store it in each vector instance.noexcept
, otherwise when placed in a standard container such asstd::vector
, they'll be copied on resize instead of being moved; see How to enforce move semantics when a vector grows?length_
is non-zero. I would zero it. Same applies to the future move constructor. On a side note, the copy constructor an assignment can (and should) be= default;
ed.std::ceil
with floating-point division looks odd. It'll give inaccurate results at large sizes, and I expect it to be slower than the integral equivalent:(a + b - 1) / b
.bit_vector::bit_vector(std::string_view s)
does something weird. It interprets the string as bytes, and takes the LSB of each character. It probably should reject everything except'0'
and'1'
, and there should be a separate constructor with a pair ofbool
iterators.The rest is purely nitpicking:
constexpr
is missing on a few functions: non-constoperator[]
and the assignment operators.-Wextra-semi
would give a bunch of warnings on;
s after closing braces.