drop asserts contradicted by the non-positive count guards - #323
Open
Ramya-9353 wants to merge 1 commit into
Open
Ramya-9353 wants to merge 1 commit into
Ramya-9353 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repro: build with asserts enabled (
cmake -DBUILD_TESTING=ON -DCMAKE_BUILD_TYPE=Debug, thenctest): test_bignum aborts onAssertion failed: (exponent >= 0), function MultiplyByPowerOfTenand test_dtoa on(mode == SHORTEST || mode == SHORTEST_SINGLE || requested_digits >= 0), function DoubleToAscii. Every Debug job of the ci workflow, and the scons job (built with -UNDEBUG), has failed this way on master since #318 landed; both workflows only run on push, so the PR checks never showed it.Cause: #318 changed MultiplyByPowerOfTen, AssignPowerUInt16 and DoubleToAscii to handle a negative count with an early return and added tests for it, but left the DOUBLE_CONVERSION_ASSERTs that reject the same value directly above the new guards, so with asserts enabled the guard is unreachable and the tests abort. GenerateCountedDigits has the same assert above the count <= 0 guard from #314.
Fix: drop the four asserts so the guard beneath each one is the single behaviour in both build types. Non-negative counts are untouched; the #318 tests now pass in Debug and Release, shared and static, under ctest and the direct cctest run.