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

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

Merged
merged 3 commits into from
Jun 26, 2022

Conversation

marco-langer
Copy link
Contributor

@marco-langer marco-langer commented May 1, 2022

Description

Closes #482

Tasklist

@codecov
Copy link

codecov bot commented May 1, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Good point!

@mloskot
Copy link
Member

mloskot commented May 20, 2022

@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 #676

@marco-langer
Copy link
Contributor Author

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
Copy link
Member

mloskot commented Jun 6, 2022

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

@marco-langer marco-langer changed the title WIP: deprecate apply_operation in favour of variant2::visit for any_image refactor: Deprecate apply_operation in favor of variant2::visit for any_image Jun 26, 2022
@marco-langer
Copy link
Contributor Author

@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.

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Awesome work!

The two CI failures on Windows are due to object size, so it's about configuration issue and we can ignore it for now:

D:\a\gil\boost-root\libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.cpp:
   fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj
    call "bin.v2\standalone\msvc\msvc-14.3\msvc-setup.bat"  >nul
 cl /Zm800 -nologo "libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.cpp" -c 
    -Fo"bin.v2\libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.test\msvc-14.3\dbg\cxstd-14-iso\thrd-mlt\copy_and_convert_pixels.obj"
    -TP /wd4675 /EHs /std:c++14 /GR /Zc:throwingNew /Z7 /Od /Ob0 /W3 /MDd /Zc:forScope /Zc:wchar_t /Zc:inline /favor:blend -DBOOST_ALL_NO_LIB=1 -D_SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING=1 "-I." "-Ilibs\gil\test" 
...failed compile-c-c++ bin.v2\libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.test\msvc-14.3\dbg\cxstd-14-iso\thrd-mlt\copy_and_convert_pixels.obj...


D:\a\gil\boost-root\libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.cpp:
  fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj
    call "bin.v2\standalone\msvc\msvc-14.3\msvc-setup.bat"  >nul
 cl /Zm800 -nologo "libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.cpp" -c
    -Fo"bin.v2\libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.test\msvc-14.3\dbg\cxstd-17-iso\thrd-mlt\copy_and_convert_pixels.obj"
    -TP /wd4675 /EHs /std:c++17 /GR /Zc:throwingNew /Z7 /Od /Ob0 /W3 /MDd /Zc:forScope /Zc:wchar_t /Zc:inline /favor:blend -DBOOST_ALL_NO_LIB=1 "-I." "-Ilibs\gil\test" 
...failed compile-c-c++ bin.v2\libs\gil\test\extension\dynamic_image\algorithm\copy_and_convert_pixels.test\msvc-14.3\dbg\cxstd-17-iso\thrd-mlt\copy_and_convert_pixels.obj...

I will stick this to Jamfile

<toolset>msvc:<cxxflags>/bigobj

@mloskot mloskot added cat/refactoring Any nonfunctional changes ext/dynamic_image boost/gil/extension/dynamic_image labels Jun 26, 2022
Avoid test\extension\dynamic_image\algorithm\copy_and_convert_pixels.cpp
compilation error:

fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj
@mloskot mloskot merged commit bfed3de into boostorg:develop Jun 26, 2022
@mloskot mloskot added this to the Boost 1.80 milestone Jun 26, 2022
mloskot added a commit that referenced this pull request Jun 27, 2022
* develop:
  docs!: Announce plan to require C++17 after Boost 1.80 (#694)
  feat: Added apply_rasterizer() free function (#695)
  refactor: Ellipse rasterizer according to the comment at (#692)
  refactor: Deprecate apply_operation in favor of variant2::visit for any_image (#656)
  refactor: Replace deprecated libtiff v4.3 typedefs with C99 fixed-size integers (#685)
  fix: Automatic detection of <filesystem> header (#684)
  test: Add tiled TIFF test case to simple_all_formats
  test: Add tests for RGB to HSL (#691)
  refactor: Move RGB to HSL tests to color_convert_rgb.cpp
  refactor: Make with_tolerance reusable across other tests
  chore: Correct include guard
  fix: Add missing #include <array>
  fix: Wrong RGB -> HSL convertion (#505)
@mloskot mloskot mentioned this pull request Jul 5, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/refactoring Any nonfunctional changes ext/dynamic_image boost/gil/extension/dynamic_image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate apply_operation and replace with variant visitation
3 participants