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

WIP: Use return type deduction to support lambda function #227

Closed
wants to merge 1 commit into from

Conversation

sdebionne
Copy link
Contributor

Allows to use gil variant (e.g. any_image<> ) visitation with generic lambdas:

gil::apply_operation(any_image_view<...>, [](auto view) {
   // do something with the view
});

Tasklist

  • Add test case(s)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

#define GIL_GENERATE_APPLY_FWD_OPS(N) BOOST_PP_REPEAT(N, GIL_APPLY_FWD_OP, BOOST_PP_EMPTY)

namespace detail {
template <std::size_t N> struct apply_operation_fwd_fn {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not increases the indentation level within namespace.
https://github.com/boostorg/gil/blob/develop/CONTRIBUTING.md#guidelines


// unary application
template <typename Types, typename Bits, typename Op>
auto BOOST_FORCEINLINE apply_operation_basec(const Bits& bits, std::size_t index, Op op) {
Copy link
Member

@mloskot mloskot Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we try to keep the lines shorter, this way?

BOOST_FORCEINLINE
auto apply_operation_basec(const Bits& bits, std::size_t index, Op op)
{
    ...
}

(@sdebionne I've just updated the snippet above)

BTW, there have been some discussion about possibility to get rid of BOOST_FORCEINLINE one day.

}

namespace detail {
template <typename T2, typename Op>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not increases the indentation level within namespace.
https://github.com/boostorg/gil/blob/develop/CONTRIBUTING.md#guidelines


// Binary application by applying on each dimension separately
template <typename Types1, typename Types2, typename Bits1, typename Bits2, typename Op>
static auto BOOST_FORCEINLINE apply_operation_base(const Bits1& bits1, std::size_t index1, const Bits2& bits2, std::size_t index2, Op op) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write your code to fit within 90 columns of text
https://github.com/boostorg/gil/blob/develop/CONTRIBUTING.md#guidelines

#else

#define GIL_APPLY_FWD_OP(z, N, text) \
template <> struct apply_operation_fwd_fn<BOOST_PP_ADD(N,1)> { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I once had a wish to get rid of BOOST_PP_* :)

} \
template <typename Types, typename Bits, typename UnaryOp> \
auto applyc(const Bits& bits, std::size_t index, UnaryOp op) const { \
typedef typename mpl::begin<Types>::type \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to replace all the MPL stuff with MP11 (http://boost.org/libs/mp11/) ?

This is a new code and it would help if it does not introduce any more uses of MPL which I'm trying to get rid of.

Even if MP11requires a bit of temporary plumbing e.g. to map boost::{true|false}_ to std::{true|false}_type, I think it is worth to try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try but this is not really new code, it's mostly copy-and-paste and getting rid of the explicit return type, letting C++14 return type deduction do the work when available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I mean, it's worth a try, but I don't think it's a show-stopper, if you don't.

@mloskot mloskot added ext/dynamic_image boost/gil/extension/dynamic_image cat/enhancement Improvements, but not fixes addressing identified bugs labels Jan 29, 2019
@mloskot mloskot requested a review from stefanseefeld January 29, 2019 12:05
@mloskot mloskot added the status/work-in-progress Do NOT merge yet until this label has been removed! label Jan 29, 2019
@sdebionne
Copy link
Contributor Author

I'll fix the formatting issues (actually I think I have a less redondant solution) but I have a question regarding the CI: are there any tests that run with std=C++14? My modifications are only active when BOOST_NO_CXX14_RETURN_TYPE_DEDUCTION is set and hence depends on that compile options.

@mloskot
Copy link
Member

mloskot commented Feb 4, 2019

@sdebionne

Are there any tests that run with std=C++14?

No, we currently don't have any builds for C++14 mode.

I think it would be a good idea to add one, and for C++17 too.

  • Do you have any preference regarding CI service?

  • Do you need a specific compiler version? See table in the README for what we've got on which CI. On CircleCI, we have these available without any extra installation required:

    gil/.circleci/config.yml

    Lines 15 to 75 in 7722f08

    docker_gcc48: &docker_gcc48
    docker:
    - image: gcc:4.8
    docker_gcc49: &docker_gcc49
    docker:
    - image: gcc:4.9
    docker_gcc51: &docker_gcc51
    docker:
    - image: gcc:5.1
    docker_gcc52: &docker_gcc52
    docker:
    - image: gcc:5.2
    docker_gcc53: &docker_gcc53
    docker:
    - image: gcc:5.3
    docker_gcc54: &docker_gcc54
    docker:
    - image: gcc:5.4
    docker_gcc55: &docker_gcc55
    docker:
    - image: gcc:5.5
    docker_gcc61: &docker_gcc61
    docker:
    - image: gcc:6.1
    docker_gcc62: &docker_gcc62
    docker:
    - image: gcc:6.2
    docker_gcc63: &docker_gcc63
    docker:
    - image: gcc:6.3
    docker_gcc64: &docker_gcc64
    docker:
    - image: gcc:6.4
    docker_gcc71: &docker_gcc71
    docker:
    - image: gcc:7.3
    docker_gcc72: &docker_gcc72
    docker:
    - image: gcc:7.2
    docker_gcc73: &docker_gcc73
    docker:
    - image: gcc:7.3
    docker_gcc82: &docker_gcc82
    docker:
    - image: gcc:8.2
    docker_clang39: &docker_clang39
    docker:
    - image: rferraro/cxx-clang:3.9
    docker_clang40: &docker_clang40
    docker:
    - image: rferraro/cxx-clang:4.0
    docker_clang50: &docker_clang50
    docker:
    - image: rferraro/cxx-clang:5.0

  • Keep in mind, Travis CI usually starts building with looong delays.

Let me know and I will add new build job, then you can update your PR.

@sdebionne
Copy link
Contributor Author

Superseded by #231.

@mloskot

Do you have any preference regarding CI service?

Nope, whatever is easier for you.

Do you need a specific compiler version? See table in the README for what we've got on which CI. On CircleCI, we have these available without any extra installation required:

According to compiler support, anything above gcc 5 is fully c++14 compliant. Personally I use gcc 7.3 as it's the one that ship with Conda but that is not a requirement.

Azure pipelines look nice and reactive.

@sdebionne sdebionne closed this Feb 5, 2019
@mloskot
Copy link
Member

mloskot commented Feb 5, 2019

OK, I will try to work it out today.

@mloskot mloskot added status/duplicate and removed status/work-in-progress Do NOT merge yet until this label has been removed! labels Feb 5, 2019
@sdebionne sdebionne deleted the apply-operation-lambda branch October 20, 2020 16:40
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 ext/dynamic_image boost/gil/extension/dynamic_image status/duplicate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants