store huffman table elements in forward order #94
Conversation
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
- Coverage 98.91% 95.80% -3.12%
==========================================
Files 11 11
Lines 277 286 +9
==========================================
Hits 274 274
- Misses 3 12 +9
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
b4e4d80
to
bc94c5c
Compare
oliverlee
force-pushed
the
Ic0e5224850d8478aa92806a8f21248e979acce04
branch
from
9db71f1
to
d1c8b2e
Compare
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
bc94c5c
to
6912c7f
Compare
oliverlee
changed the base branch from
master
to
If703d0b0a6307ebcf9e64722431069247ba51800
oliverlee
changed the base branch from
master
to
If703d0b0a6307ebcf9e64722431069247ba51800
oliverlee
changed the base branch from
If703d0b0a6307ebcf9e64722431069247ba51800
to
master
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
6912c7f
to
debbdb9
Compare
huffman/src/table.hpp
Outdated
| using C [[maybe_unused]] = std::common_type_t<std::make_unsigned_t<S>, U>; | ||
|
|
||
| assert( | ||
| static_cast<C>(uint) < static_cast<C>(std::numeric_limits<S>::max())); |
There was a problem hiding this comment.
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
debbdb9
to
72620db
Compare
| for (; first != last; first = first->next()) { | ||
| using S = std::ranges::range_difference_t<decltype(table_)>; | ||
|
|
||
| assert(std::cmp_less_equal(uint, std::numeric_limits<S>::max())); |
There was a problem hiding this comment.
Why std::cmp_less_equal instead of <=?
There was a problem hiding this comment.
There's a comparison of signed with unsigned integral types and I don't know the integral promotion and conversion rules. With cmp_less_equal, it "will do the right thing". If both operands were signed or both were unsigned integrals, I would use <=. I also partially rely on the compiler to catch these issues with -Wconversion
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wconversion
There's some decent motivation in the original proposal to add these functions:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0586r2.html
| } | ||
| // postcondition | ||
| assert(total_freq == table_.front().frequency()); | ||
| std::ranges::reverse(table_); |
There was a problem hiding this comment.
I'll add this later:
If front().frequency() does not equal total_freq, we have skipped over nodes during construction of the implicit Huffman tree.
There was a problem hiding this comment.
Or you feel free to :)
There was a problem hiding this comment.
I meant comment about why the reverse, not about the assertion.
|
Hmm test coverage is down! Did you add functions we don't need to static_vector or table? |
I only added |
|
Hmm yeah probably an artifact of how we're calculating things. |
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
72620db
to
1f290ae
Compare
|
Failing code coverage is due to lack of tests for tables with zero symbols and one symbol |
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
1f290ae
to
44ba57c
Compare
Remove use of std::reverse_iterator when iterating over table elements. Instead, invoke reverse on the underlying table storage after construction. Change-Id: I45916a591f51298fff8b4012b9507b781021100e
oliverlee
force-pushed
the
I45916a591f51298fff8b4012b9507b781021100e
branch
from
44ba57c
to
5bac1df
Compare
Remove use of std::reverse_iterator when iterating over table elements.
Instead, invoke reverse on the underlying table storage after
construction.
Change-Id: I45916a591f51298fff8b4012b9507b781021100e