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

Make assert configurable via JSON_ASSERT #2242

Merged
merged 6 commits into from
Jul 9, 2020
Merged

Make assert configurable via JSON_ASSERT #2242

merged 6 commits into from
Jul 9, 2020

Conversation

nlohmann
Copy link
Owner

@nlohmann nlohmann commented Jul 6, 2020

This PR introduces a macro JSON_ASSERT(x) which defaults to assert(x), but can be replaced by any other implementation.

Closes #2239.

@coveralls
Copy link

coveralls commented Jul 6, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling f774a32 on issue2239 into efcc826 on develop.

@nlohmann
Copy link
Owner Author

nlohmann commented Jul 6, 2020

It seems there is a memory issue - Valgrind and ASAN are complaining about the new test case.

@nlohmann nlohmann marked this pull request as draft July 6, 2020 20:53
@gregmarr
Copy link
Contributor

gregmarr commented Jul 7, 2020

I think any assert still needs to terminate the process somehow, or you're going to have issues with the algorithms not working properly because the preconditions are violated. That means that you can't really test that an assert fires.

@nlohmann
Copy link
Owner Author

nlohmann commented Jul 8, 2020

I think any assert still needs to terminate the process somehow, or you're going to have issues with the algorithms not working properly because the preconditions are violated. That means that you can't really test that an assert fires.

I think I chose a bad function to test - one where a non-aborting assertion left a non-void function without return value. I now chose a different function where first an assertion fires and even if it does not abort an exception is thrown.

@nlohmann
Copy link
Owner Author

nlohmann commented Jul 8, 2020

(The test fails if exceptions are switched off...)

@nlohmann nlohmann marked this pull request as ready for review July 9, 2020 06:37
@nlohmann nlohmann self-assigned this Jul 9, 2020
@nlohmann nlohmann added this to the Release 3.8.1 milestone Jul 9, 2020
@nlohmann nlohmann merged commit 4c7bd01 into develop Jul 9, 2020
@nlohmann nlohmann deleted the issue2239 branch July 9, 2020 13:13
@nlohmann
Copy link
Owner Author

nlohmann commented Jul 9, 2020


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


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.

3 participants