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

msvc: scripted-diff: Remove NDEBUG pre-define in project file #15431

Merged
merged 2 commits into from Feb 17, 2019

Conversation

Projects
None yet
5 participants
@ken2812221
Copy link
Member

commented Feb 17, 2019

Follow #15391

ken2812221 added some commits Feb 17, 2019

scripted-diff: Remove NDEBUG pre-define
-BEGIN VERIFY SCRIPT-
sed -i 's/NDEBUG;//g' $(git grep --name-only 'NDEBUG;' build_msvc)
-END VERIFY SCRIPT-
@fanquake

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2019

@ken2812221 isn't NDEBUG desirable for msvc release builds? Without it assert statements will terminate the programs.

Update: After looking over #15391 I now see the origin of the removal. I guess it's a design decision for the bitcoin-core programs but on Windows it's a real pain when bitcoin-qt crashes as the result of an assertion. It's very rare to see that type of behaviour on Windows apps. The preferred approach is to throw an exception and at least display a usable error message to users before terminating.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

You can't build it after #15391 merged. The CI pass because I disabled them by the appveyor.yml

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

@sipsorcery If an assertion is false, aborting is often far better than the alternative (with assumptions violated, we may accept an invalid chain for example...).

@bitcoin bitcoin deleted a comment from trebliw5 Feb 17, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

How could this compile locally, but then suddenly stop to compile with #15391, given that we have the same compile time check in validation.cpp?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

Maybe @sipsorcery just delete them in libbitcoin_server.vcxproj in his first PR?

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2019

tACK 8a1f0a3 3ec56be.

@MarcoFalke if your comment was for me then I didn't have any problems compiling with msvc before or after this PR. Instead I was just having a little moan about the unfriendly way asserts terminate bitcoin-qt. But that decision was made by a wiser person(s) than me so I'll track down the root cause instead.

@MarcoFalke MarcoFalke merged commit 3ec56be into bitcoin:master Feb 17, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Feb 17, 2019

Merge #15431: msvc: scripted-diff: Remove NDEBUG pre-define in projec…
…t file

3ec56be appveyor: Remove unused NDEBUG removal (Chun Kuan Lee)
8a1f0a3 scripted-diff: Remove NDEBUG pre-define (Chun Kuan Lee)

Pull request description:

  Follow #15391

Tree-SHA512: f264418cbc69b5f083469ed9005a6d592d4268f2b7da967e571ce30195de73b09a9e14c8610a5b6b0f056847d82a4bc7c2fbe56498307093aab4dd42903e6137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.