Fix staj_event::as_double silently returning 0 for non-numeric strings#711
Open
jsutes wants to merge 2 commits into
Open
Fix staj_event::as_double silently returning 0 for non-numeric strings#711jsutes wants to merge 2 commits into
jsutes wants to merge 2 commits into
Conversation
Regression introduced in caacd25 ("Remove chars_to, replace with to_double"): the migration replaced a throwing chars_to functor with the new out-parameter to_double API but discarded its return code, leaving the output at its default 0 with no error propagated to the caller's std::error_code&. As a result, decoding a non-numeric or empty CSV cell into a floating-point type (e.g. decode_csv<std::vector<std::vector<double>>>) silently produced 0.0 instead of reporting an error. Check the result of decstr_to_double and set ec = conv_errc::not_double on failure, matching how the same call is handled in basic_csv_parser::end_value_with_numeric_check.
The strtod-family fallbacks of decstr_to_double (used when JSONCONS_HAS_STD_FROM_CHARS is not defined, i.e. GCC < 11 and all Clang) return success for zero-length input: their error check `if (end < cur)` is unreachable when length == 0, because cur == s and end >= s always. The previous commit therefore only fixed the empty-cell case on platforms with std::from_chars. Guard length_ == 0 explicitly in as_double so an empty string is reported as not_double on every platform.
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
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.
Summary
basic_staj_event::as_doublesilently returns0.0(with no error) when astring_value/keyevent holds text that isn't a number. Any decoder thatasks a string event for a
double— e.g.decode_csv<std::vector<std::vector<double>>>overinfer_types(true)output —therefore coerces non-numeric and empty cells to
0.0instead of reporting aconversion error.
Root cause
Regression introduced in caacd25 ("Remove chars_to, replace with to_double").
Before that commit, the string branch of
as_doubleused thechars_tofunctor, which threw
json_runtime_error<std::invalid_argument>on invalidinput. The commit migrated to the new out-parameter API:
The new call reports failure through its return value, but this call site
discards it — so
valkeeps its default0and the caller'sstd::error_code&is never set. (Later renames turned
to_doubleintodecstr_to_double; thebehavioral bug is unchanged.)
For comparison,
basic_csv_parser::end_value_with_numeric_checkuses the samedecstr_to_doublecall and does checkresult.ec.Fix
include/jsoncons/staj_event.hpp,as_double:to_number_resulthas anexplicit operator bool()that is false on error, so!resultcovers bothstd::errc::invalid_argument(non-numeric / empty input)and any other failure.
Reproducer
Tests
Added
TEST_CASE("decode_csv non-numeric string into double reports error")to
test/csv/src/encode_decode_csv_tests.cpp, covering a non-numeric token(
"hey\n") and an empty trailing field ("0,\n"). Each section asserts bothtry_decode_csvreturns an error anddecode_csvthrows.master(thetry_decode_csvcall silentlysucceeds).
(883 test cases, 20,639 assertions).