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 164 forks source link

Use of min and max in Gil conflicts with Win32-API on Windows/MSVC #744

Closed ckormanyos closed 4 months ago

ckormanyos commented 4 months ago

Actual behavior

I was recently using Boost.Gil with traditional Win32-API programming on MSVC (in Windows) and discovered a few conflicts with windows.h. Unfortunately windows.h defines min() and max() macros that conflict with the standard C++ ones.

Defining NOMINMAX (or un-defining BOOST_USE_WINDOWS_H) above inclusion of Windows-headers in client code works around this.

I could PR the fix with trivial changes on 9 lines. Should I do this PR?

Or would you just prefer to leave it, sort of forcing clients to definine NOMINMAX?

Expected behavior

It might be preferable, however, to not require defining NOMINMAX.

C++ Minimal Working Example

I found 9 instances in total, one example is here.

Instead of:

static constexpr Tuple min()

use

static constexpr Tuple (min)()

Environment

MSVC 141, 142, 143 solely in combination with Boost.Gil headers and the inclusion of <windows.h>.

mloskot commented 4 months ago

Thanks @ckormanyos for reporting this. Although (min)() is a fine solution, I'm always unclear what is the Boost-wide convention to handle this annoyance. Do you perhaps know? I'd prefer to follow solution that is widely adopted by Boost developers and known to Boost users.

ckormanyos commented 4 months ago

what is the Boost-wide convention to handle this annoyance. Do you perhaps know?

We use (min)() etc. in Math and Multiprecision. When I just started boosting, I learned or thought I learned from @jzmaddock that this parenthesis-based workaround is the Boost way.

ckormanyos commented 4 months ago

I’ll just PR the trivial changes tonight and you can decide at your convenience if it’s a go or no-go.

mloskot commented 4 months ago

We use (min)() etc. in Math and Multiprecision...

I'm used to this trick as well.

I’ll just PR the trivial changes tonight

Thank you. I'm sure nobody will object it and your PR will be merged. Especially, that we had accepted similar fixes e.g. #328

BTW, AFAICT, we mix both workarounds, the () and the NOMINMAX e.i. https://github.com/boostorg/gil/commit/87b7fdbcab359f7159a639aa4e210db765d73679 The former is more portable, of course, as build system agnostic.