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 asserts with INVARIANT et al. to enable compilation with NDEBUG #1536

Closed

Conversation

andreast271
Copy link
Contributor

The changes in this PR are needed:

  • to compile unit tests with NDEBUG
  • to compile with clang with NDEBUG

clang discovered an instance of an unused variable that had been ignored by gcc in remove_returns.cpp.

@smowton
Copy link
Contributor

smowton commented Oct 30, 2017

The unit test is probably best converted to Catch if we're going to mess with it -- that shouldn't mean much more than removing the driver main function, wrapping the whole thing in a TEST_CASE block, and replacing all assert with REQUIRE. See https://github.com/philsquared/Catch/blob/master/docs/tutorial.md if you're not familiar.

The remove-return change looks reasonable, but rather than do half a job let's replace the other 8 asserts in that file if we're doing one.

@andreast271
Copy link
Contributor Author

The point of the PR is to enable a CI build with disabled assertions/invariants etc. Converting the unit test to Catch is out of scope. The same holds for the change to remove_returns.cpp: it fails to compile with clang++ -DNDEBUG, this PR fixes that. Changing assertions to invariants everywhere is worth doing, but not the purpose of this PR.

@smowton
Copy link
Contributor

smowton commented Oct 31, 2017

Suggest replacing with #1537 and #1538 which implement remove-returns assert->invariant and Catch-ifying the sharing-node test respectively.

@andreast271
Copy link
Contributor Author

I agree. This PR is now obsolete.

@smowton
Copy link
Contributor

smowton commented Oct 31, 2017

Great, I will ping you when they are merged (hopefully hours not days :)) Would you mind checking that the build is successful with develop+1537+1538 in your configuration?

@smowton
Copy link
Contributor

smowton commented Oct 31, 2017

@andreast271 those two are both now merged; latest develop should work as you intend.

@andreast271
Copy link
Contributor Author

@smowton thank you very much. I replied to your comment from 10:50 GMT because I saw a problem. I guess you never got that message ...

@smowton
Copy link
Contributor

smowton commented Oct 31, 2017

I don't see anything here between my comment 10:50 and my comment 13:15, no. Repeat it?

@andreast271 andreast271 deleted the unit-test-NDEBUG-compilation branch March 21, 2021 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants