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

pull ken2812221 wants to merge 2 commits into bitcoin:master from ken2812221:msvc-no-ndebug changing 18 files +34 −35

ken2812221 commented at 10:37 AM on February 17, 2019: contributor

Follow #15391

scripted-diff: Remove NDEBUG pre-define

-BEGIN VERIFY SCRIPT-
sed -i 's/NDEBUG;//g' $(git grep --name-only 'NDEBUG;' build_msvc)
-END VERIFY SCRIPT-

8a1f0a38d4

appveyor: Remove unused NDEBUG removal 3ec56bea0d
fanquake added the label Windows on Feb 17, 2019
fanquake added the label Build system on Feb 17, 2019
fanquake commented at 12:27 PM on February 17, 2019: member

cc @sipsorcery
sipsorcery commented at 3:35 PM on February 17, 2019: member

@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 commented at 3:42 PM on February 17, 2019: contributor

You can't build it after #15391 merged. The CI pass because I disabled them by the appveyor.yml
sipa commented at 3:49 PM on February 17, 2019: member

@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...).
MarcoFalke deleted a comment on Feb 17, 2019
MarcoFalke commented at 4:29 PM on February 17, 2019: member

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 commented at 4:40 PM on February 17, 2019: contributor

Maybe @sipsorcery just delete them in libbitcoin_server.vcxproj in his first PR?
sipsorcery commented at 5:39 PM on February 17, 2019: member

tACK 8a1f0a38d4a7cef544001e86c569bde747db2ecb 3ec56bea0d7b36bef2b6c7095ecd7d9fb43a3631. @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 merged this on Feb 17, 2019
MarcoFalke closed this on Feb 17, 2019
MarcoFalke referenced this in commit b72c787dc8 on Feb 17, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15391 Add compile time verification of assumptions we're currently making implicitly/tacitly