Skip to content
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: use std::size_t as channel index type in nth_channel_view #659

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

striezel
Copy link
Contributor

@striezel striezel commented May 4, 2022

Description

This unifies the channel index type in nth_channel_view and
image_view::num_channels - the latter one already uses std::size_t.

References

Fixes #373.

Tasklist

  • Ensure all CI builds pass
  • Review and approve

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.10%. Comparing base (322c4e2) to head (8821258).
Report is 1 commits behind head on develop.

❗ Current head 8821258 differs from pull request most recent head 0f93276. Consider uploading reports for the commit 0f93276 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #659      +/-   ##
===========================================
- Coverage    82.12%   81.10%   -1.03%     
===========================================
  Files          117      117              
  Lines         5355     5171     -184     
===========================================
- Hits          4398     4194     -204     
- Misses         957      977      +20     

@striezel
Copy link
Contributor Author

Changes have been rebased against the current develop branch.
Furthermore, std::size_t is replaced by boost::gil::index_t in the affected files. At the moment boost::gil::index_t is an alias for std::ptrdiff_t, as suggested in #373 (comment) and following comments.

However, the basic idea was to unify the channel index type, any while I could change image_view::num_channels to return boost::gil::index_t, its implementation currently uses gil::num_channels which uses boost::mp11::mp_size which is an alias for boost::mp11::mp_size_t which - you may have already guessed it by the name - finally uses std::size_t. So if we want to get rid of the signed/unsigned mixup (which I believe was the original intention behind #373), we either have to convince boost::mp11::mp_size to switch to std::ptrdiff_t (seems unlikely to me) or change boost::gil::index_t to be an alias for std::size_t.

@mloskot:
Any advice on how to proceed from here?

@mloskot
Copy link
Member

mloskot commented May 31, 2022

How about re-implementing num_channels to something along these lines https://godbolt.org/z/3b4Pz8EKE ?

#include <type_traits>
#include <boost/mp11.hpp>
using namespace boost::mp11;

struct R {};
struct G {};
struct B {};
using rgb_t = mp_list<R, G, B>; // sample color space
using index_t = std::ptrdiff_t;

template <typename L>
struct num_channels
{
    using type = typename mp_size<L>::type;
    static constexpr index_t value = mp_size<L>::value;
};

static_assert(num_channels<rgb_t>::value == 3, "");

@striezel striezel force-pushed the unified-channel-index-type branch 2 times, most recently from f0109b4 to d745375 Compare June 28, 2022 20:48
@mloskot
Copy link
Member

mloskot commented Jun 29, 2022

@striezel I've just noticed this PR received updates. What is the status of it? Do you want me to squeeze it for Boost 1.80? Today is a deadline for major changes.

@striezel
Copy link
Contributor Author

@mloskot:

Do you want me to squeeze it for Boost 1.80? Today is a deadline for major changes.

No.
The deadline passed yesterday, so I guess it's not really an option to squeeze it into Boost 1.80 anymore. But even if it were still possible, I am not really happy with the current state of this branch / pull request. So for the moment it is better to leave this PR out of the next release.

The two issues which I see are:

  • It has merge conflicts that need to be resolved.
    I did not have the time to resolve them the day before yesterday, so I just rebased against a more recent commit that still has no merge conflicts to avoid falling too far behind. But this definitely needs a rebase against the most current commit on the develop branch, and I will probably do this in the next days. It's just not in time for Boost 1.80 anymore, so there is no need to rush it and squeeze it in somehow.
  • Now that boost::gil::index_t is the way to go, some more places need to change from std::size_t or int (or whatever they are using) to boost::gil::index_t. This includes basically any use of nth_channel_view and num_channels within GIL, including tests.

I don't see that I will be able to do this within the next one or two days, so there really is no need to rush this PR into Boost 1.80. It will be in Boost 1.81 then, I guess.

@mloskot
Copy link
Member

mloskot commented Jun 30, 2022

I don't see that I will be able to do this within the next one or two days, so there really is no need to rush this PR into Boost 1.80. It will be in Boost 1.81 then, I guess.

Sure. No rush, no pressure, no worry. I'm very thankful for all your contributions.

@mloskot mloskot added this to the Boost 1.81+ milestone Jul 7, 2022
@mloskot mloskot added the cat/enhancement Improvements, but not fixes addressing identified bugs label Jul 7, 2022
@striezel
Copy link
Contributor Author

striezel commented Sep 1, 2022

The two issues which I see are:

  • It has merge conflicts that need to be resolved.
    I did not have the time to resolve them the day before yesterday, so I just rebased against a more recent commit that still has no merge conflicts to avoid falling too far behind. But this definitely needs a rebase against the most current commit on the develop branch, and I will probably do this in the next days. It's just not in time for Boost 1.80 anymore, so there is no need to rush it and squeeze it in somehow.

  • Now that boost::gil::index_t is the way to go, some more places need to change from std::size_t or int (or whatever they are using) to boost::gil::index_t. This includes basically any use of nth_channel_view and num_channels within GIL, including tests.

Rebase is done (force push from d745375 to 5fa0eb8).

Next: Find other places where std::size_t or int need to be replaced by boost::gil::index_t.

@mloskot mloskot modified the milestones: Boost 1.82, Boost 1.83+ Mar 31, 2023
@striezel striezel force-pushed the unified-channel-index-type branch from 8821258 to 0f93276 Compare May 5, 2024 17:33
@mloskot mloskot modified the milestones: Boost 1.86, 1.87.0 Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/enhancement Improvements, but not fixes addressing identified bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify channel index type in nth_channel_view and image_view::num_channels
2 participants