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

Replace Boost.MPL with Boost.MP11 #274

Merged
merged 1 commit into from Apr 14, 2019

Conversation

@mloskot
Copy link
Member

mloskot commented Apr 2, 2019

Description

  • Use type traits and features of C++11, then use Boost.MP11.
  • Remove unused and unnecessary metafunctions in detail namespace.
  • Remove explicit access to ::type as no longer necessary with MP11.
  • Replace boost::is_integral with gil::detail::is_channel_integral to avoid UB.
  • Clean up and reformat code according to the current guidelines.

Legacy tests have been updated where necessary to accommodate switch to MP11.

Status

Ready to start review process.

I will try make it easier to review/track changes and submit updates as new commits,
but sometimes I may need to rebase and force-push update. Sorry.

Two files have not been updated and it needs to be decided what to do:

  • promote_integral.hpp which is external source borrowed from Boost.Geometry.
  • extension/dynamic_image/reduce.hpp which is compiled on demand by #define BOOST_GIL_REDUCE_CODE_BLOAT=1

References

  • Among others, implements #93
  • Closes #229

Tasklist

  • Decide what to do with promote_integral.hpp. See #280
  • Decide what to do with extension/dynamic_image/reduce.hpp. See #281
  • Solve failing checksum test in test/legacy/image.cpp for bgr121_* (fixed in f45dd04)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed
@mloskot mloskot self-assigned this Apr 2, 2019
@mloskot mloskot added this to To Do in C++11 Modernization via automation Apr 2, 2019
@mloskot mloskot requested review from stefanseefeld and chhenning Apr 2, 2019
@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 9, 2019

The failing checksum test should now be fixed in f45dd04

It's funny how implicit namespace can make refactoring a bit more difficult :-)
Simply, old boost::is_integral vs new std::is_integral led to call to completely different channel_converter_unsigned_impl specialization than expected.

@stefanseefeld

This comment has been minimized.

Copy link
Member

stefanseefeld commented Apr 9, 2019

You make me curious. Please explain !

@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 9, 2019

@stefanseefeld Let's try. Below, I'm using references to the current implementation based on Boost.MPL so it should be easier to follow.

Consider this template:

template <typename SrcChannelV, typename DstChannelV> // Model ChannelValueConcept
struct channel_converter_unsigned
: public detail::channel_converter_unsigned_impl<SrcChannelV,DstChannelV,is_integral<SrcChannelV>::value,is_integral<DstChannelV>::value> {};

Notice, this template which uses unqualified is_integral.

For the bgr121_* test cases in the legacy/image.cpp, SrcChannelV is unsigned char and DstChannelV is packed_channel_value.

Now, for the packed_channel_value, there is this boost::is_integral specialization

namespace boost {
template <int NumBits>
struct is_integral<gil::packed_channel_value<NumBits> > : public mpl::true_ {};

which is 'called' during specialization of base channel_converter_unsigned_impl template presented earlier.

The Boost.MPL to Boost.MP11 convesrion in this PR replaces the boost::is_integral with std::is_integral and the specializations are now in std namespace, but that unqualified 'call' to is_integral as parameter to channel_converter_unsigned_impl above still referred to boost::is_integral.
Since there was no specialization of it for the packed_channel_value, the result was false instead of true and it led to using this default definition of channel converter:

/// \brief This is the default implementation. Performance specializatons are provided
template <typename SrcChannelV, typename DstChannelV, bool SrcIsIntegral, bool DstIsIntegral>
struct channel_converter_unsigned_impl {
using argument_type = SrcChannelV;
using result_type = DstChannelV;
DstChannelV operator()(SrcChannelV src) const {
return DstChannelV(channel_traits<DstChannelV>::min_value() +
(src - channel_traits<SrcChannelV>::min_value()) / channel_range<SrcChannelV>() * channel_range<DstChannelV>());
}

instead of this

template <typename SrcChannelV, typename DstChannelV>
struct channel_converter_unsigned_impl<SrcChannelV,DstChannelV,true,true>
: public channel_converter_unsigned_integral<SrcChannelV,DstChannelV,
mpl::less<unsigned_integral_max_value<SrcChannelV>,unsigned_integral_max_value<DstChannelV> >::value > {};

that, for the pair of channels in the bgr121_* tests (SrcChannelV is unsigned char and DstChannelV is packed_channel_value) eventually calls:

template <typename SrcChannelV, typename DstChannelV>
struct channel_converter_unsigned_integral_impl<SrcChannelV,DstChannelV,false,true> {
DstChannelV operator()(SrcChannelV src) const {
using integer_t = typename unsigned_integral_max_value<SrcChannelV>::value_type;
static const integer_t div = unsigned_integral_max_value<SrcChannelV>::value / unsigned_integral_max_value<DstChannelV>::value;
static const integer_t div2 = div/2;
return DstChannelV((src + div2) / div);
}
};

I hope it makes sense :-)

@mloskot mloskot force-pushed the mloskot:ml/replace-boost-mpl-with-mp11 branch 2 times, most recently from 25ab1d8 to 2e4d7c7 Apr 10, 2019
@Kojoley

This comment has been minimized.

Copy link
Contributor

Kojoley commented Apr 10, 2019

That's not my business, but I wonder how you are going review this ;)

P.S. There are bunch of changes like replacing boost with std type_traits (mpl::true_/false_ also in this category) or silencing warnings that could be done separately.

@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 11, 2019

@Kojoley

I wonder how you are going review this ;)

It was well understood MPL to MP11 will propose large changeset that may be difficult if not impossible to carefully review diff by diff.
That is why I've spent significant amount of time updating/adding tests prior to this work.
That is why I've added so many CI builds for as many compilers and versions as possible.

This is a big jump and, as every thorough refactoring of non-trivial codebase, it will introduce issues. So, we must continue adding more tests and more detailed tests with hope to fish for the issues - we will spend 3, 6, 9, ... months before we decide we are happy to release it.

Finally, despite of the risks, I believe this does not deteriorate any GIL qualities; on the contrary.

"You want to change things, you have to break things" :-)

There are bunch of changes like replacing boost with std type_traits (mpl::true_/false_ also in this category) or silencing warnings that could be done separately.

I attempted to remove only those traits or some other traits (e.g. #107), but due to tight coupling of MPL and Boost.TypeTraits (and tight coupling of GIL to those two), it was not practically feasible or required some 'mapping' of traits to C++11 equivalents which would have been clear/remove-refactored again once switching to MP11.

I fished for some places where it was possible to apply C++11 features w/o breaking existing MPL-based infrastructure (e.g. #215, #225 and some more). Regardless, number of places where C++11 traits could be used directly was insignificant in comparison to size of the overall changeset required anyway.


I'm always advocate surgical strikes at code that introduce manageable diffs, but sometimes it is not feasible (esp. w/ limited time and manpower).

@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 11, 2019

Build Status Update

This PR passes all these builds (compilers info in README.md):

  • AppVeyor (C++14 w/ Boost develop)
  • AzP (C++11 and C++14 w/ Boost 1.68)
  • CircleCI (C++11 w/ Boost master)
  • TravisCI (C++11 w/ Boost develop)

The only failing build is the AzP one for C++17 w/ Boost 1.68 using VS2017:

3>c:\boost\include\boost-1_68\boost\mp11\detail\mp_plus.hpp(28): error C2059: syntax error: '...' [D:\a\1\s\_build\test\legacy\legacy_pixel_iterator.vcxproj]
         c:\boost\include\boost-1_68\boost\mp11\detail\mp_fold.hpp(47): note: see reference to class template instantiation 'boost::mp11::detail::mp_plus_impl<V,T1>' being compiled
                 with
                 [
                     V=std::integral_constant<int,0>,
                     T1=std::integral_constant<size_t,2>
                 ]
         c:\boost\include\boost-1_68\boost\mp11\detail\mp_fold.hpp(47): note: see reference to alias template instantiation 'boost::mp11::mp_plus<std::integral_constant<int,0>,T1>' being compiled
                 with
                 [
                     T1=std::integral_constant<size_t,2>
                 ]
         d:\a\1\s\include\boost\gil\bit_aligned_pixel_reference.hpp(123): note: see reference to class template instantiation 'boost::mp11::detail::mp_fold_impl<ChannelBitSizes,std::integral_constant<int,0>,boost::mp11::mp_plus>' being compiled
                 with
                 [
                     ChannelBitSizes=boost::mp11::mp_list<std::integral_constant<size_t,2>,std::integral_constant<size_t,3>,std::integral_constant<size_t,2>>
                 ]
         d:\a\1\s\include\boost\gil\bit_aligned_pixel_reference.hpp(118): note: see reference to alias template instantiation 'boost::mp11::mp_fold<boost::mp11::mp_list<std::integral_constant<size_t,2>,std::integral_constant<size_t,3>,std::integral_constant<size_t,2>>,std::integral_constant<int,0>,boost::mp11::mp_plus>' being compiled

I'm suspecting this is a problem with MP11 from the older release of Boost 1.68.
If this is confirmed, then I will disable this build until AzP builds switch to Boost 1.70 (once released).

Results

There is an issue in MP11 from Boost 1.68 related to fold expressions (thanks to @pdimov for confirmation). Here is MWE:

#include <boost/version.hpp>
static_assert(BOOST_VERSION == 106800, "");
#include <boost/mp11.hpp>
#include <cstdint>
#include <type_traits>
int main()
{
    using V = std::integral_constant<int, 0>;
    using T1 = std::integral_constant<std::size_t, 2>;
    using R = boost::mp11::mp_plus<V,T1>;
    R r;
    return 0;
}

image

Plan

Update the Azure Pipelines builds to follow the grand rule of "test develop against develop and master against master", as the AppVeyor builds do.

@mloskot mloskot mentioned this pull request Apr 11, 2019
1 of 1 task complete
@mloskot mloskot force-pushed the mloskot:ml/replace-boost-mpl-with-mp11 branch from 316cde8 to 3e633ad Apr 11, 2019
@mloskot mloskot mentioned this pull request Apr 12, 2019
3 of 3 tasks complete
Use type traits and features of C++11, then use Boost.MP11.
Remove unused and unnecessary metafunctions in `detail` namespace.
Remove explicit access to ::type as no longer necessary with MP11.
Clean up and reformat code according to the current guidelines.

Legacy tests have been updated where necessary to accommodate
switch to MP11.

Replace std::is_integral with gil::detail::is_channel_integral
Replacing boost::is_integral with std::is_integral is C++ UB:

    C++11 / 20.11.2 Header <type_traits> synopsis
    1 The behavior of a program that adds specializations for any
    of the class templates defined in this subclause is undefined
    unless otherwise specified.
@mloskot mloskot force-pushed the mloskot:ml/replace-boost-mpl-with-mp11 branch from 3e633ad to 4617dd6 Apr 13, 2019
@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 13, 2019

I've rebased the PR onto develop to completed the plan from #274 (comment) and integrate the latest CI update from #279

Copy link
Member

stefanseefeld left a comment

This is fantastic. Thanks for all your hard work on this !

@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 14, 2019

I think I'll merge tonight :) Thrilled!

@mloskot mloskot merged commit 5611bd5 into boostorg:develop Apr 14, 2019
9 checks passed
9 checks passed
boostorg.gil Build #20190413.2 succeeded
Details
boostorg.gil (macos1013_xcode91_cmake) macos1013_xcode91_cmake succeeded
Details
boostorg.gil (ubuntu1604_gcc5_cxx11_cmake) ubuntu1604_gcc5_cxx11_cmake succeeded
Details
boostorg.gil (ubuntu1604_gcc8_cxx14_cmake) ubuntu1604_gcc8_cxx14_cmake succeeded
Details
boostorg.gil (win2012_vs2015_cmake) win2012_vs2015_cmake succeeded
Details
boostorg.gil (win2016_vs2017_cxx14_cmake) win2016_vs2017_cxx14_cmake succeeded
Details
boostorg.gil (win2016_vs2017_cxx17_cmake) win2016_vs2017_cxx17_cmake succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
C++11 Modernization automation moved this from To Do to Done Apr 14, 2019
@mloskot mloskot deleted the mloskot:ml/replace-boost-mpl-with-mp11 branch Apr 14, 2019
@mloskot

This comment has been minimized.

Copy link
Member Author

mloskot commented Apr 14, 2019

Ka-Boom!

@mloskot mloskot added this to the Boost 1.71 milestone Apr 15, 2019
mloskot added a commit that referenced this pull request Apr 18, 2019
Lost in 5611bd5 from #274 merge
Credit for catching this goes to @stefanseefeld
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.