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 limit macros with std::numeric_limits #3723

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

Replace uses of INT_MIN/INT_MAX, etc. with std::numeric_limits and consistently use std namespaced integer types.

Fixes #3722.

@github-actions
Copy link

github-actions bot commented Sep 4, 2022

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated.

@falbrechtskirchinger
Copy link
Contributor Author

silkeh/clang:dev either doesn't install clang-tools anymore or scan-build isn't found for some other reason.

    apt-get install -qqy -t llvm-toolchain-bullseye \
        clang clang-tidy clang-format lld llvm && \

Not sure why this worked previously. Maybe clang-tools was pulled in as a dependency.

@coveralls
Copy link

coveralls commented Sep 4, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling de9d0f8 on falbrechtskirchinger:numeric_limits into 58bd97e on nlohmann:develop.

@setoye
Copy link

setoye commented Sep 12, 2022

There is a problem if you do in this way. In a Windows platform if you just #include <Windows.h> before your json library, a syntax error with std::numeric_limits::max() will occur because of the notorious macro definition in this Windows.h header file:

#define max(a,b) (((a) > (b)) ? (a) : (b))

and this piece of code will expand into

std::numeric_limits<std::int32_t>::(((a) > (b)) ? (a) : (b))

Which is an error.
Add a pair of parentheses will prevent the macro expansion:

(std::numeric_limits<std::int32_t>::max)()

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Sep 12, 2022

Thanks. I was already aware of this. There's no point in fixing this now. This PR needs to be completely redone due to hundreds of merge conflicts after #3724 has been merged.

Add a pair of parentheses will prevent the macro expansion:

(std::numeric_limits<std::int32_t>::max)()

This is already done in other places for this exact reason. The only surprising thing is that our unit test didn't catch this.

Edit: Not that surprising after all. doctest.h defines NOMINMAX.

@falbrechtskirchinger
Copy link
Contributor Author

Windows builds are expected to fail now.

@falbrechtskirchinger
Copy link
Contributor Author

🤦‍♂️

@falbrechtskirchinger falbrechtskirchinger changed the title Replace limit macros and use namespaced int types Replace limit macros with std::numeric_limits Sep 18, 2022
@nlohmann nlohmann added this to the Release 3.11.3 milestone Sep 19, 2022
@nlohmann nlohmann merged commit 3d1252b into nlohmann:develop Sep 19, 2022
@nlohmann
Copy link
Owner

Thanks!

@falbrechtskirchinger falbrechtskirchinger deleted the numeric_limits branch September 19, 2022 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INT64_MIN/MAX not defined for newer g++
5 participants