boostorg / gil

Boost.GIL - Generic Image Library | Requires C++14 since Boost 1.80
https://boostorg.github.io/gil
Boost Software License 1.0
178 stars 163 forks source link

refactor: Deprecate apply_operation in favor of variant2::visit for any_image #656

Closed marco-langer closed 2 years ago

marco-langer commented 2 years ago

Description

Closes #482

Tasklist

codecov[bot] commented 2 years ago

Codecov Report

Merging #656 (85f01f9) into develop (151fd9c) will increase coverage by 0.37%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #656      +/-   ##
===========================================
+ Coverage    80.32%   80.69%   +0.37%     
===========================================
  Files          117      116       -1     
  Lines         5032     5072      +40     
===========================================
+ Hits          4042     4093      +51     
+ Misses         990      979      -11     
sdebionne commented 2 years ago

I think the idea of #482 was just to mark apply_operation as [[deprecated]] to encourage users to use visit directly in user code. I am not sure removing apply_operation from the codebase is a good idea, especially if we switch to C++17 in the near future -then std::variant will be used.

Thanks for the tests of the dynamic algorithms!

marco-langer commented 2 years ago

Thank you for the feedback!

I might be wrong, but I think we need to do both at the same time: mark apply_operation() as deprecated and remove all internal use cases at the same time. Otherwise the deprecated warning will be emitted even if the caller never invokes apply_operation() himself. From the compilers point of view it is irrelevant, whether the deprecated function will be called from within the library or from client code.

Please let me know if this assumption is not correct though.

sdebionne commented 2 years ago

Otherwise the deprecated warning will be emitted even if the caller never invokes apply_operation() himself.

Good point!

mloskot commented 2 years ago

@sdebionne

if we switch to C++17 in the near future -then std::variant will be used.

Let's do it. I have moved the planning towards C++14/17 to discussion here https://github.com/boostorg/gil/discussions/676

marco-langer commented 2 years ago

With the announcement to change to std::variant with C++17, starting from Boost 1.82, as I understood the discussion, shall we still take this intermediate step here and switch to boost::variant2::visit during for Boost 1.80 and 1.81, or should this issue be postponed to 1.82?

mloskot commented 2 years ago

@marco-langer I don't see any objections to the intermediate step.

marco-langer commented 2 years ago

@mloskot This PR is ready for review. I kept the WIP tag in the last weeks to complete/refactor the tests, but the core library refactoring is done. I think we can add/refactor the tests also after the Boost 1.80 deadline.