-
Notifications
You must be signed in to change notification settings - Fork 164
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
fix: is_equal_to_sixteen in PNG I/O was less-than test #650
fix: is_equal_to_sixteen in PNG I/O was less-than test #650
Conversation
While is_equal_to_sixteen should (as the name suggests) check for equality to 16, it actually checked whether the bit depth was less than 16.
Codecov Report
@@ Coverage Diff @@
## develop #650 +/- ##
===========================================
- Coverage 78.77% 78.76% -0.01%
===========================================
Files 117 117
Lines 5031 5030 -1
===========================================
- Hits 3963 3962 -1
Misses 1068 1068 |
Good catch, @striezel ! It looks like I did a copy & paste mistake copying Hmm, since this bug was introduced by #274 in Boost 1.72 which is
as @striezel points out in #633 (comment), so any chance it is related to the issue #633 ? |
Well, that is what I am hoping for. Fixing |
Seems like it. I just made another test with the example from #633 (comment). Using Boost from the current master branch (boostorg/boost@0f43af9) still has the weird colors in the 16 bit image. However, when I switch the submodule for GIL to the version from this PR, then the 16 bit image comes out fine. |
Sounds good, thank you @striezel for your help |
Sorry for the late feedback but shouldn't
|
Yes, there is no requirement to use mp11 features here. |
As I understand it,
(from https://en.cppreference.com/w/cpp/types/is_same) In contrast to that
(from https://www.boost.org/doc/libs/1_79_0/libs/mp11/doc/html/mp11.html#mp_lesst1_t2) To summarize it, the mp11 stuff compares values while So this would not be the same as the implementation that got merged. Or am I missing something here? |
Note that I am not saying your implementation does not work, it's just a bit over complicated. template <typename Info>
struct is_equal_to_sixteen : std::is_same
<
std::integral_constant<int, Info::_bit_depth>,
std::integral_constant<int, 16>
>
{}; |
Description
While
is_equal_to_sixteen
should (as the name suggests) check for equality to 16, it actually checked whether the bit depth was less than 16.Since Boost.MP11 does not contain an
mp11::mp_equal
but onlymp11::mp_less
, the equality check is performed by making sure thatIt's a bit laborious, but it works.
References
The wrong comparison was introduced by PR #274 in 5611bd5#diff-e2c958c778b464b94146992b6aeb7c814839ddbcbbb901f9f4175e797fa543d2 when
boost::mpl
was replaced withboost::mp11
. In that commit the originalmpl::equal_to
was replaced bymp11::mp_less
. To be fair though, unlike MPL the MP11 library does not contain an equivalent tompl::equal_to
.Tasklist