Refine integer conversions in unsigned_integral_max_value

[mloskot]

As per Andrey Semashev's suggestion https://lists.boost.org/Archives/boost/2018/04/242013.php,

since we switched to C++11, it may be a good idea to

• use the standard constexpr std::numeric_limits<UnsignedIntegralChannel>::max()
• remove dependenc on #include <boost/integer_traits.hpp>

Comments?

[T-Deuty]

This seems to require the NOMINMAX preprocessor definition. I'd suggest adding that with the check for if its already defined before wherever windows.h is included, if in fact it is included.

Perhaps it isn't in the GIL library and its just my large project, but I'm running into issues when I need both this to compile and code using the standard MAX macro.

Edit:
Adding this link to a separate issue where the same problem I'm trying to describe is brought up: boostorg/stacktrace#76

[mloskot]

@T-Deuty

This seems to require the NOMINMAX preprocessor definition

Yes, Windows SDK/MSVC, in many, if not most uses of std::numeric_limits<T>::max() will either require to #define NOMINMAX or function-like macro suppression with extra parentheses (std::numeric_limits<T>::max)() or one of several other possible tricks to work around the Windows SDK/MSVC annoyance.

I'd suggest adding that with the check for if its already defined before wherever windows.h is included, if in fact it is included.

AFAICT, nowhere in GIL the windows.h is included. However, to ensure building tests/examples is not prone to this annoyance, we #define NOMINMAX in the build configurations:

gil/test/Jamfile

Line 41 in c3e84ad

<toolset>msvc:<define>NOMINMAX

gil/CMakeLists.txt

Line 88 in c3e84ad

$<$<CXX_COMPILER_ID:MSVC>:NOMINMAX>

IMO, it is not GIL or Boost or other C++ library duty but a library's user duty to #define NOMINMAX before any Windows header, as suggested by LLVM/clang/libcxx infrastructure:

#ifdef min
#if !defined(_LIBCPP_DISABLE_MACRO_CONFLICT_WARNINGS)
#if defined(_LIBCPP_WARNING)
_LIBCPP_WARNING("macro min is incompatible with C++.  Try #define NOMINMAX "
                "before any Windows header. #undefing min")
#else
#warning: macro min is incompatible with C++.  #undefing min
#endif
#endif
#undef min
#endif

Alternatively, we may consider adding very similar check and warning somewhere in boost/gil.hpp. What do you think, @stefanseefeld ?

[T-Deuty]

Thanks for getting back to me so quickly. I agree with your opinion that it is on the library user to handle this, I'll look into defining NOMINMAX for my project. I was doing so until I came across an error where we're using what I think is a standard definition of max, in another library, that won't compile with NOMINMAX defined. So feels like a catch 22 between using GIL and this other library.

We're using Boost-gil 1.70 from Vcpkg by the way. My vote would be for the "function-like macro suppression with extra parenthesis" but in the meantime I'll try to find all the Windows headers and define NOMINMAX before including them.

Thanks,
Tyler

[mloskot]

My vote would be for the "function-like macro suppression with extra parenthesis"

I've searched through the code and apparently GIL already uses this technique in numerous places, e.g.

gil/include/boost/gil/color_convert.hpp

Lines 147 to 148 in c3e84ad

	get_color(dst,black_t()) = (std::min)(get_color(dst,cyan_t()),
	(std::min)(get_color(dst,magenta_t()),

as well as explicit template parameter to suppress the macro:

gil/include/boost/gil/algorithm.hpp

Line 975 in c3e84ad

std::ptrdiff_t const num = std::min<std::ptrdiff_t>(n, i2.width() - i2.x_pos());

Please, consider contributing a pull request with similar fixes that will help your project.
Since such technique is already in use, I don't see any reason why we shouldn't accept it.