MacOS already defines `MIN` and `MAX`

[TrueBrain]

First of all, thank you for this library. It is rare to find a library, especially for cryptographic operations, where the documentation is so easy to follow, that implementing it is trivial. I especially like the clear opinion about what to do and what not to do, and having examples you can just copy/paste to get going. Compared to other libraries, this really was a relief. So thank you.

That said, while integrating this into OpenTTD, I found one small annoyance: on MacOS, at least within our configuration, the macro MIN and MAX are already defined by MacOS:

monocypher/monocypher.cpp:69:9: warning: 'MIN' macro redefined [-Wmacro-redefined]
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/sys/param.h:215:9: note: previous definition is here

As such, including monocypher.h generates the above warning. It still works, nothing is broken, etc, but it disallows treating warnings as errors.

For now, we went for: just rename MIN to MC_MIN, and done. But maybe there is a more elegant solution here. As I don't have access to a Mac, I can't actually check what the MIN macro is, but maybe it already does what monocypher defines it at. ( https://github.com/OpenTTD/OpenTTD/commit/7294af9bcc0bde88e3a9f037411f76e1df1d354a for reference )

Anyway, just wanted to let you know. Tnx again for this library :)

[LoupVaillant]

Hi, I'm glad to see how much you appreciate our work. You can thank @fscoto for the whole documentation philosophy, as well as half the documentation.

As such, including monocypher.h generates the above warning.

Hmm, looking at your error, what's actually happening is that for some reason, /usr/include/sys/param.h figures among the transitive dependencies of monocypher.cpp. I'm not sure why, this shouldn't be happening from just including stdint.h and stddef.h (the only two headers Monocypher depends on), since params.h doesn't seem to appear in the source code I could find. My guess here is that something in your build system is causing this extraneous inclusion.

For now, we went for: just rename MIN to MC_MIN, and done. But maybe there is a more elegant solution here.

Two things might be worth exploring here:

• Compiling Monocypher as C instead of C++. The hope is that the extraneous inclusion wouldn't happen with C files.
• Disabling -Werror just for monocypher.cpp and monocypher-ed25519.cpp, and argue that -Werror should not apply to third party code. Note that including monocypher.h should not trigger any warning, since it defines no macro.

If none of those work, your patch looks good. I would gladly accept it if it turns out this is a more general issue on MacOSX. For now however it seems something specific to your project is going on. It would be nice if we could confirm it one way or another.

[TrueBrain]

Thank you for the detailed analysis: I kinda completely forgot we use precompiled headers to speed up compiling. So all source-files get a bunch of STL headers included, whether they include it or not. And it is very likely that one of them includes param.h. That indeed would make it very specific to our project.

So I do apologize, as I could have realised that earlier :) If you do receive more reports about this, I have no problems PRing our change, but as PCH is not all that common, that might not be the case :)

Tnx for your time!

[LoupVaillant]

Glad to hear you found the root cause!

At a first glance, I would guess that using pre-compiled headers for third party code may not be the best idea. Not just because of the errors they might introduce, but because third party code is rarely modified, and as such is unlikely to benefit from PCH.

So it looks like the most elegant solution here is to disable PCH on third party code. If you can. Otherwise your patch is perfectly valid.

[TrueBrain]

Excellent suggestion, thank you! Disabling PCH would indeed be better here instead of patching the code :)