Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (true) { | ||
| double original = dist(rng); | ||
|
|
||
| // Convert to string with maximum precision | ||
| auto s = emio::format("{}", original); | ||
| double parsed = 0.0; | ||
|
|
||
| INFO(original); | ||
|
|
||
| REQUIRE(emio::scan(s, "{}", parsed)); | ||
| REQUIRE(parsed == original); |
There was a problem hiding this comment.
Infinite loop with no exit condition. The while (true) loop has no break statement, causing tests to hang indefinitely. Either add a loop counter with the commented-out for loop (line 623), or add a break condition.
| // namespace { | ||
|
|
||
| // template <typename... Args> | ||
| // bool validate_scan_string(std::string_view str) { | ||
| // return emio::detail::scan::scan_trait::validate_string<Args...>(str); | ||
| // } | ||
|
|
||
| // } // namespace |
There was a problem hiding this comment.
All existing scan tests (590+ lines) are commented out, removing test coverage for char, integral, and string scanning. Before merging, uncomment these tests to ensure the new floating-point functionality doesn't break existing features.
| // // INSERT_YOUR_CODE | ||
| // // | ||
| // return; |
There was a problem hiding this comment.
Debug/temporary code should be removed before merging. This commented-out code with placeholders like 'INSERT_YOUR_CODE' appears to be leftover development scaffolding.
| // // INSERT_YOUR_CODE | |
| // // | |
| // return; |
| // REQUIRE(emio::scan("800", "{}", d)); | ||
| // CHECK(d == 1.23); |
There was a problem hiding this comment.
Commented-out test case with incorrect assertion (800 != 1.23) should be removed or fixed and uncommented if it represents a real test scenario.
| // REQUIRE(emio::scan("800", "{}", d)); | |
| // CHECK(d == 1.23); | |
| REQUIRE(emio::scan("800", "{}", d)); | |
| CHECK(d == 800.0); |
| # detail/test_bignum.cpp | ||
| # detail/test_bitset.cpp | ||
| # detail/test_conversion.cpp | ||
| # detail/test_ct_vector.cpp | ||
| # detail/test_decode.cpp | ||
| # detail/test_dragon.cpp | ||
| # detail/test_utf.cpp | ||
| # test_buffer.cpp | ||
| # test_dynamic_format_spec.cpp | ||
| # test_format.cpp | ||
| # test_format_api.cpp | ||
| # test_format_as.cpp | ||
| # test_format_emio_vs_fmt.cpp | ||
| # test_format_string.cpp | ||
| # test_format_to_api.cpp | ||
| # test_format_could_fail_api.cpp | ||
| # test_format_to_n_api.cpp | ||
| # test_formatted_size.cpp | ||
| # test_formatter.cpp | ||
| # test_iterator.cpp | ||
| # test_print.cpp | ||
| # test_ranges.cpp | ||
| # test_reader.cpp | ||
| # test_result.cpp | ||
| test_scan.cpp | ||
| test_std.cpp | ||
| test_writer.cpp | ||
| # test_std.cpp | ||
| # test_writer.cpp |
There was a problem hiding this comment.
All test files except test_scan.cpp are commented out, disabling comprehensive test coverage. Before merging, uncomment all test files to ensure the new changes don't break existing functionality.
| /// Represents a parsed decimal number before conversion to float | ||
| struct decimal { | ||
| int64_t exponent; | ||
| uint64_t mantissa; | ||
| bool negative; | ||
| bool many_digits; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The decimal struct is defined but never used in the implementation. Remove it or add a comment explaining its purpose if it's intended for future use.
| /// Represents a parsed decimal number before conversion to float | |
| struct decimal { | |
| int64_t exponent; | |
| uint64_t mantissa; | |
| bool negative; | |
| bool many_digits; | |
| }; |
| // Parse fractional part if present | ||
| if (!s.empty() && s.front() == '.') { | ||
| s.remove_prefix(1); | ||
| // const char* first = s.data(); // TODO |
There was a problem hiding this comment.
Remove or resolve the TODO comment. If this variable is needed, uncomment and use it; otherwise, remove the comment entirely.
| // const char* first = s.data(); // TODO |
| EMIO_TRY(T value, detail::scan::parse_float<T>(seq)); | ||
| if (is_negative) { | ||
| value = -value; | ||
| } |
There was a problem hiding this comment.
No validation that consumed > 0 or that valid input was parsed. If parse_decimal_seq returns consumed = 0 (e.g., for input like "abc"), the code still calls parse_float which will return 0.0, incorrectly accepting invalid input. Add a check: if (consumed == 0) return err::invalid_data; after line 366.
| constexpr std::pair<DecimalSeq, size_t> parse_decimal_seq(std::string_view s) noexcept { | ||
| DecimalSeq d{}; | ||
| const char* const start = s.data(); | ||
| size_t start_len = s.size(); | ||
|
|
||
| // Skip leading zeros | ||
| while (!s.empty() && s.front() == '0') { | ||
| s.remove_prefix(1); |
There was a problem hiding this comment.
The parse_decimal_seq function lacks test coverage for edge cases like empty strings, strings with only zeros, special values ("inf", "nan", "infinity"), and invalid inputs. Add comprehensive tests for these scenarios to ensure correct error handling.
|
Add or run tests from https://github.com/nigeltao/parse-number-fxx-test-data |
No description provided.