Fix 30 bugs found during code review by Centimo · Pull Request #618 · getml/reflect-cpp

Centimo · 2026-02-22T21:11:43Z

Fix 30 bugs found during code review

Summary

This MR fixes 30 bugs across core types, JSON, XML, TOML, MsgPack, BSON, Avro, Cap'n Proto, and CLI modules. Each bug has a regression test that fails before the fix and passes after.

Stats: 39 files changed, 1197 insertions, 52 deletions.

Discussion: Breaking API Changes

The following 3 fixes change public API behavior. They are correct from a safety standpoint, but may break downstream code. Please review whether a deprecation period or alternative approach is preferred.

1. `Validator::get()`/`value()` return mutable reference

Breaking change: Removes non-const overloads of get(), value(), operator*(), operator()().
Code like validator.get() = new_value will no longer compile.

Conflicts with API convention: PR #613 established that wrapper types should provide operator*, .value(), .get(), and operator(). This fix removes the non-const variants to prevent mutation that bypasses validation. An alternative would be to keep non-const overloads but route them through re-validation (i.e. a validating setter).

File: include/rfl/Validator.hpp:98-99, 114-115

Problem: Non-const get(), value(), operator*(), operator()() return T&, allowing direct write that bypasses validation.

Test: tests/generic/test_validator.cpp — validator_get_does_not_allow_invalid_mutation. Uses std::is_assignable_v to verify get() returns const T&.

Fix: Removed all non-const overloads (get(), value(), operator*(), operator()()).

2. `Literal(const std::string&)` is implicit

Breaking change: Adds explicit to the string constructor.
Code like rfl::Literal<"a","b"> lit = my_string; will no longer compile; must use rfl::Literal<"a","b">(my_string) instead.

File: include/rfl/Literal.hpp:46

Problem: Non-explicit constructor allows implicit std::string → Literal conversion, which throws on invalid input. Surprising for users.

Test: tests/generic/test_literal.cpp — literal_constructor_is_explicit. Uses std::is_convertible_v to verify implicit conversion is disabled.

Fix: Added explicit keyword.

3. `TaggedUnion` — typo `discrimininator_`

Breaking change: Renames the public static member discrimininator_ → discriminator_.
Code referencing TaggedUnion<...>::discrimininator_ will no longer compile.
A [[deprecated]] alias could be added to ease migration if desired.

File: include/rfl/TaggedUnion.hpp:15

Problem: Extra 'n' — discrimininator_ instead of discriminator_.

Test: tests/generic/test_tagged_union.cpp — tagged_union_discriminator_spelling. Uses concepts to check member name existence.

Fix: Renamed to discriminator_. Also fixed the typo in the error message string literal "discrimininator" → "discriminator" in Parser_tagged_union.hpp:151.

Bugs Fixed

4. `Tuple::operator<=>` always returns `equivalent`

File: include/rfl/Tuple.hpp:107

Problem: The condition _ordering != equivalent is checked on first iteration where _ordering is initialized to equivalent — the body never executes. The operator returns equivalent for any two tuples.

Test: tests/generic/test_tuple.cpp — tuple_spaceship_not_always_equivalent, tuple_spaceship_less. Verifies Tuple<int>(1) <=> Tuple<int>(2) returns less, not equivalent.

Fix: Changed != to == so the comparison body executes when ordering is still undetermined.

5. `Result::operator=(const Result&)` does not update `success_`

File: include/rfl/Result.hpp:210-216

Problem: The converting assignment copies t_or_err_ but never updates success_. After assigning an error Result into a value Result<T>, success_ remains true — .value() reads garbage.

Test: tests/generic/test_result.cpp — result_cross_type_assign_error_clears_success. Assigns error Result<long> to value Result<int>, checks the target is falsy.

Fix: Replaced raw union copy with proper destroy() + success_ update + move_from_other().

6. `Validator::operator=(U&&)` is `noexcept` but throws

File: include/rfl/Validator.hpp:77

Problem: The operator calls .value() which throws std::runtime_error on validation failure. Marked noexcept — any validation error calls std::terminate.

Test: tests/generic/test_validator.cpp — validator_assign_rvalue_operator_is_noexcept_but_can_throw. Uses std::is_nothrow_assignable_v to verify the operator is no longer noexcept.

Fix: Removed noexcept specifier.

7. `Generic::to_int()` silently truncates `int64_t` to `int`

File: include/rfl/Generic.hpp:175-188

Problem: static_cast<int>(_v) with no range check. Values outside [INT_MIN, INT_MAX] are silently truncated.

Test: tests/generic/test_generic.cpp — generic_to_int_rejects_overflow, generic_to_int_rejects_underflow. Passes INT_MAX+1 and INT_MIN-1, expects failure.

Fix: Added range check before the cast, returning an error for out-of-range values.

8. `Box`/`Ref` `operator<=>` compares pointer addresses

Files: include/rfl/Box.hpp:130, include/rfl/Ref.hpp:118

Problem: _b1.ptr() <=> _b2.ptr() compares memory addresses instead of pointed-to values. Two Box<int>(42) with different allocations compare as unequal.

Test: tests/generic/test_box.cpp — box_spaceship_compares_values_not_pointers. tests/generic/test_ref.cpp — ref_spaceship_compares_values_not_pointers.

Fix: Changed to *_b1 <=> *_b2 (dereference before compare).

9. `Field`/`Flatten`/`Skip` cross-type move constructor copies instead of moving

Files: include/rfl/Field.hpp:37, include/rfl/Flatten.hpp:32, include/rfl/internal/Skip.hpp:43

Problem: value_(_field.get()) — get() returns lvalue reference, so the copy constructor is called instead of move.

Test: tests/generic/test_field.cpp — field_cross_type_move_does_not_copy. tests/generic/test_flatten.cpp — flatten_cross_type_move_does_not_copy. tests/generic/test_skip.cpp — skip_cross_type_move_does_not_copy. Use MoveTracker types that count copies vs moves.

Fix: Changed to value_(std::move(_field.get())) in all three types.

10. `Result::value_or(T&&)` returns dangling rvalue reference

File: include/rfl/Result.hpp:295

Problem: Returns T&& instead of T. If the default argument is a temporary, the caller gets a dangling reference.

Test: tests/generic/test_result.cpp — result_value_or_returns_value_not_rvalue_ref. Checks return type is T, not T&&.

Fix: Changed return type from T&& to T.

11. `Result::error() &&` returns `Error&` instead of `Error&&`

File: include/rfl/Result.hpp:335

Problem: The rvalue-qualified overload returns Error& but get_err() && returns Error&&. Prevents move semantics on rvalue Results.

Test: tests/generic/test_result.cpp — result_error_rvalue_qualified_returns_rvalue_ref. Checks return type is Error&&.

Fix: Changed return type from Error& to Error&&.

12. `Object::difference_type` is unsigned

File: include/rfl/Object.hpp:29

Problem: using difference_type = typename DataType::size_type — size_type is unsigned, but the C++ standard requires difference_type to be signed.

Test: tests/generic/test_object.cpp — object_difference_type_is_signed. Uses std::is_signed_v.

Fix: Changed to typename DataType::difference_type.

13. `transform_camel_case` prepends `_` to names starting with uppercase

File: include/rfl/internal/transform_case.hpp:60-73

Problem: When _i == 0 and the first character is uppercase, the function unconditionally prepends '_'. "MyField" becomes "_my_field" instead of "my_field".

Test: tests/json/test_regression_bugs.cpp — camel_case_to_snake_case_no_leading_underscore. Serializes a struct with CamelCaseToSnakeCase and checks output.

Fix: Added sizeof...(chars) > 0 guard so '_' is only inserted between characters, not at the beginning.

14. `Timestamp::to_time_t()` double-subtracts timezone offset

File: include/rfl/Timestamp.hpp:120

Problem: timegm(&tm) - tm_.tm_gmtoff — timegm() already returns UTC. Subtracting tm_gmtoff applies the timezone correction twice. Non-Windows only.

Test: tests/json/test_regression_bugs.cpp — timestamp_to_time_t_no_double_timezone_correction. Sets tm_gmtoff = 3600 and verifies to_time_t() returns the correct UTC epoch.

Fix: Removed - tm_.tm_gmtoff.

15. `FieldVariantParser` — wrong error message

File: include/rfl/parsing/FieldVariantParser.hpp:47-50

Problem: When zero matching fields are found, the error says "found more than one" instead of "found none".

Test: tests/json/test_regression_bugs.cpp — field_variant_empty_object_error_message. Parses "{}" and checks the error message does not contain "more than one".

Fix: Changed message to "found none".

16. `json::Writer` reads past `string_view` bounds

File: src/rfl/json/Writer.cpp:68, :91, :113

Problem: yyjson_mut_strcpy(_name.data()) reads until \0, but string_view::data() is not null-terminated. Reads past buffer for substrings.

Test: tests/json/test_regression_bugs.cpp — json_writer_handles_non_null_terminated_field_names. Creates a string_view substring and verifies the JSON key matches the view's length.

Fix: Changed all three call sites from yyjson_mut_strcpy to yyjson_mut_strncpy with explicit size.

17. `cli::Reader` — `stoull` accepts negative numbers for unsigned types

File: include/rfl/cli/Reader.hpp:77

Problem: std::stoull("-1") wraps to ULLONG_MAX without error. static_cast<uint16_t> wraps again to 65535.

Test: tests/cli/test_regression_bugs.cpp — cli_rejects_negative_for_unsigned. Passes --port=-1 and expects failure.

Fix: Added explicit check for leading - character, returning an error for negative input.

18. `cli::Reader` — no range check on unsigned narrowing cast

File: include/rfl/cli/Reader.hpp:77

Problem: static_cast<T>(stoull(str)) silently truncates. stoull("99999") for uint16_t produces 34463.

Test: tests/cli/test_regression_bugs.cpp — cli_rejects_out_of_range_for_narrow_type. Passes --port=99999 for uint16_t and expects failure.

Fix: Added std::numeric_limits<T>::max() bounds check before the cast.

19. `cli::Reader` — no range check on signed narrowing cast

File: include/rfl/cli/Reader.hpp:101-102

Problem: static_cast<T>(stoll(str)) silently truncates for int8_t/int16_t/int32_t. stoll("200") cast to int8_t wraps to -56.

Test: tests/cli/test_regression_bugs.cpp — cli_rejects_out_of_range_for_signed_narrow_type, cli_rejects_large_negative_for_signed_narrow_type. Passes --level=200 and --level=-200 for int8_t and expects failure.

Fix: Added std::numeric_limits<T>::min()/max() bounds check before the cast.

20. `cli::Reader` — `std::stod` is locale-dependent

File: include/rfl/cli/Reader.hpp:64

Problem: std::stod uses the C locale. In locales with comma decimal separator (e.g. de_DE), "3.14" parses as 3.0.

Test: tests/cli/test_regression_bugs.cpp — cli_float_parsing_ignores_locale. Sets de_DE locale, parses --rate=3.14, checks result is 3.14.

Fix: Replaced std::stod with std::from_chars which is locale-independent.

21. `capnproto::read(istream, schema)` — infinite recursion

File: include/rfl/capnproto/read.hpp:79-82

Problem: The overload read(std::istream&, const Schema<T>&) calls read<T, Ps...>(_stream, _schema) — resolves to itself. Stack overflow.

Test: tests/capnproto/compile_fail/capnproto_read_istream_schema.cpp — instantiates the overload. Before the fix, GCC rejects it as "use of auto before deduction" (infinite recursion detected at compile time). After the fix, the file compiles successfully, verifying the overload resolves correctly.

Fix: Read bytes from stream into a vector, then call read(bytes.data(), bytes.size(), _schema).

22. `avro::SchemaImpl` move-assignment — double-decrement / use-after-free

File: src/rfl/avro/SchemaImpl.cpp:34-42

Problem: Move-assignment neither releases the old iface_ (leak) nor nullifies _other.iface_ (double-decrement when _other's destructor runs).

Test: tests/avro/test_regression_bugs.cpp — schema_impl_move_assignment_no_double_free. Move-assigns two SchemaImpl objects, verifies source iface_ is null after move.

Fix: Added avro_value_iface_decref/avro_schema_decref for the old iface before reassignment, and _other.iface_ = nullptr after.

23. `xml::Reader` uses `stoi` for all integer types

File: include/rfl/xml/Reader.hpp:99-105

Problem: std::stoi returns int (32-bit). For int64_t/uint64_t fields, values beyond INT_MAX throw out_of_range or are silently truncated.

Test: tests/xml/test_regression_bugs.cpp — read_int64_above_int32_max, read_int64_large_negative, read_uint64_large_value. Parse XML with values exceeding 32-bit range.

Fix: Use std::stoll for signed types and std::stoull for unsigned types.

24. `toml::Writer` — `at_path()` dereferences as path, crashes on dotted keys

File: src/rfl/toml/Writer.cpp:26, :40

Problem: After emplace("a.b", ...), at_path("a.b") interprets the dot as a path separator and looks for table["a"]["b"] which doesn't exist — returns invalid node — as_table()/as_array() returns nullptr.

Test: tests/toml/test_regression_bugs.cpp — field_name_with_dot_does_not_crash, array_field_name_with_dot_does_not_crash. Use rfl::Rename<"a.b", ...> to create dotted field names.

Fix: Replaced at_path(_name) with operator[](_name) which does literal key lookup.

25. `msgpack::read` ignores `msgpack_unpack` return value

File: include/rfl/msgpack/read.hpp:35-36

Problem: If msgpack_unpack fails, the return value is not checked. deserialized remains uninitialized — garbage is parsed, producing misleading downstream errors like "Could not cast to a map".

Test: tests/msgpack/test_regression_bugs.cpp — read_single_invalid_byte_returns_meaningful_error, read_truncated_data_returns_meaningful_error. Pass invalid/truncated data and verify the error doesn't mention "cast".

Fix: Check return value; return "Failed to unpack msgpack data." error on failure. Accept both MSGPACK_UNPACK_SUCCESS and MSGPACK_UNPACK_EXTRA_BYTES.

26. `msgpack::Writer` stores `uint64_t` as signed `int64`

File: include/rfl/msgpack/Writer.hpp:133-134

Problem: All integers are packed via msgpack_pack_int64(static_cast<int64_t>(...)). For uint64_t values above INT64_MAX, the cast wraps to negative — msgpack stores them as NEGATIVE_INTEGER.

Test: tests/msgpack/test_regression_bugs.cpp — uint64_above_int64_max_stored_as_positive, uint64_max_stored_as_positive. Verify raw msgpack wire type is POSITIVE_INTEGER.

Fix: Added a separate std::is_unsigned branch that calls msgpack_pack_uint64.

27. `bson::Reader` rejects integer BSON values for float fields

File: include/rfl/bson/Reader.hpp:127-132

Problem: The float branch checks btype != BSON_TYPE_DOUBLE and rejects everything else — even though the error message claims int32/int64/date_time are valid.

Test: tests/bson/test_regression_bugs.cpp — read_int64_into_double_field, read_large_int64_into_double_field. Write an int struct, read it back as a double struct.

Fix: Changed to a switch accepting BSON_TYPE_DOUBLE, INT32, INT64, and DATE_TIME.

28. `delete`/`delete[]` mismatch in test

File: tests/json/test_pointer_fields.cpp

Problem: A raw pointer field allocated with new[] was freed with delete instead of delete[].

Test: Pre-existing test; fix prevents undefined behavior under sanitizers.

Fix: Changed delete to delete[].

29. `Flatten(U&&)` universal reference constructor copies instead of forwarding

File: include/rfl/Flatten.hpp:40

Problem: value_(_value) — _value is a named parameter (lvalue), so the copy constructor is always called even when an rvalue is passed. Should be value_(std::forward(_value)).

Test: tests/generic/test_flatten.cpp — flatten_universal_ref_ctor_forwards_rvalue. Constructs Flatten<FlatBase> from FlatDerived&& and verifies rvalue conversion is used.

Fix: Changed to value_(std::forward(_value)).

30. `Flatten::operator=(Flatten&&)` forwards `Flatten` instead of `U`

File: include/rfl/Flatten.hpp:103-105

Problem: std::forward(_f) forwards the Flatten object, not U. Assignment value_ = Flatten looks for implicit conversion which uses get() (lvalue ref) — copy instead of move. Causes hard compile error for types without Flatten → Type conversion.

Test: tests/generic/compile_fail/flatten_cross_type_move_assign.cpp — compile test that instantiates the cross-type move assignment. Verifies compilation succeeds after the fix.

Fix: Changed to value_ = std::move(_f.get()).

Tests cover bugs in Tuple, Result, Validator, Generic, Box, Ref, Field, TaggedUnion, Timestamp, FieldVariantParser, CamelCaseToSnakeCase, and CLI Reader. All tests intentionally fail to confirm bug presence.

- Tuple::operator<=>: fix inverted condition (!= → ==) - Result::operator=(const Result&): properly update success_ flag - Validator::operator=(U&&): remove incorrect noexcept - Generic::to_int(): add range check for int64_t → int cast - Validator: remove mutable get()/value()/operator*()/operator()() - Box/Ref::operator<=>: compare values instead of pointers - Field cross-type move ctor: add std::move to avoid copy - Result::value_or: return T by value instead of T&& - TaggedUnion: fix typo discrimininator_ → discriminator_ - transform_camel_case: prevent leading underscore - Timestamp::to_time_t: remove double timezone correction - FieldVariantParser: fix error message for empty object - cli::Reader: reject negative values and check range for unsigned - Parser_tagged_union: fix discriminator typo in error message

Tests for bugs that could not be fixed (require third-party library changes or are design-level issues): - Result::error() && returns Error& instead of Error&& (2.4) - Object::difference_type is unsigned instead of signed (3.8) - Literal(const std::string&) is implicit, can throw unexpectedly (3.5) - Flatten/Skip cross-type move copies instead of moving (3.9) - Parser_string_view/Parser_span leak memory on every read (1.13) - cli::Reader std::stod is locale-dependent (2.7) Memory leak tests use a separate executable (json_alloc) with custom operator new[]/delete[] to track allocations without ASAN.

- Result::error() &&: return Error&& instead of Error& - Object::difference_type: use signed DataType::difference_type - Literal: make string constructor explicit - Flatten: std::move in cross-type move constructor - Skip: std::move in cross-type move constructor - cli::Reader: use std::from_chars instead of locale-dependent std::stod

- Disable alloc memory leak tests (detect known unfixed leaks in Parser_string_view and Parser_span) - Fix UB in test_pointer_fields: delete -> delete[] for memory allocated with new char[]

yyjson_mut_strcpy reads until '\0', but std::string_view::data() does not guarantee null-termination. Replace with yyjson_mut_strncpy which respects the explicit length. Add regression test.

Tests for: capnproto read(istream,schema) infinite recursion, avro SchemaImpl move-assignment double-free, xml Reader stoi truncation for 64-bit types, toml Writer at_path nullptr with dotted keys, msgpack read ignoring unpack errors, bson Reader rejecting ints for float fields, msgpack Writer storing uint64 as signed int64.

- avro: SchemaImpl move-assignment now releases old iface_ and nullifies source, preventing double-decrement/use-after-free - bson: Reader float branch accepts INT32/INT64/DATE_TIME BSON types, matching the error message and Writer behavior - capnproto: read(istream, schema) reads bytes from stream instead of calling itself recursively - msgpack: read() checks msgpack_unpack return value, returning early error instead of parsing uninitialized data - msgpack: Writer uses msgpack_pack_uint64 for unsigned types, preventing uint64 values above INT64_MAX from being stored as negative - toml: Writer uses operator[] instead of at_path() for key lookup, fixing nullptr dereference with dotted field names - xml: Reader uses stoll/stoull instead of stoi for 64-bit integer types, fixing truncation of values beyond INT32 range

- Flatten(U&&): use std::forward instead of copying - Flatten::operator=(Flatten&&): use std::move(_f.get()) instead of std::forward(_f) - cli::Reader signed parse_value: add range check before static_cast<T>

Move 18 tests from monolithic test_regression_bugs.cpp into dedicated test files (test_tuple, test_result, test_validator, test_generic, test_field, test_skip, test_object) and existing component files (test_box, test_ref, test_flatten, test_literal, test_tagged_union). Extract shared helper types into test_helpers.hpp.

gemini-code-assist · 2026-02-22T21:12:43Z

Summary of Changes

Hello @Centimo, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses and resolves 30 identified bugs across various core types and serialization modules including JSON, XML, TOML, MsgPack, BSON, Avro, and Cap'n Proto. The fixes enhance correctness, improve type safety, prevent undefined behavior, and ensure more robust data handling. Several changes also address potential breaking API behaviors, which are explicitly documented for review.

Highlights

Validator::get()/value() return mutable reference: Removed non-const overloads of get(), value(), operator*(), operator()() to prevent mutation that bypasses validation. This is a breaking change.
Literal(const std::string&) is implicit: Added 'explicit' to the string constructor of rfl::Literal to prevent implicit conversions that could throw. This is a breaking change.
TaggedUnion — typo discrimininator_: Renamed the public static member 'discrimininator_' to 'discriminator_' in TaggedUnion and its error message. This is a breaking change.
Tuple::operator<=> always returns equivalent: Corrected the comparison logic in Tuple::operator<=> to properly compare elements instead of always returning 'equivalent'.
Result::operator=(const Result&) does not update success_: Fixed converting assignment in Result to correctly update the 'success_' flag and handle internal union state.
Validator::operator=(U&&) is noexcept but throws: Removed the 'noexcept' specifier from Validator::operator=(U&&) as it could throw on validation failure, preventing std::terminate.
Generic::to_int() silently truncates int64_t to int: Added range checks to Generic::to_int() to prevent silent truncation of int64_t values when converting to int.
Box/Ref operator<=> compares pointer addresses: Modified Box and Ref operator<=> to compare the pointed-to values instead of memory addresses.
Field/Flatten/Skip cross-type move constructor copies instead of moving: Updated cross-type move constructors for Field, Flatten, and Skip to correctly use std::move for their underlying values.
Result::value_or(T&&) returns dangling rvalue reference: Changed Result::value_or(T&&) to return T by value instead of T&& to prevent dangling references.
Result::error() && returns Error& instead of Error&&: Corrected Result::error() && to return Error&&, enabling proper move semantics for rvalue Results.
Object::difference_type is unsigned: Changed Object::difference_type to be signed, aligning with the C++ standard requirement.
transform_camel_case prepends _ to names starting with uppercase: Fixed transform_camel_case to avoid prepending an underscore to field names that start with an uppercase letter.
Timestamp::to_time_t() double-subtracts timezone offset: Removed redundant timezone offset subtraction in Timestamp::to_time_t() to ensure correct UTC epoch conversion.
FieldVariantParser — wrong error message: Corrected the error message in FieldVariantParser when zero matching fields are found, changing it from "found more than one" to "found none".
json::Writer reads past string_view bounds: Updated json::Writer to use yyjson_mut_strncpy with explicit size for string_view data, preventing reads past buffer bounds.
cli::Reader — stoull accepts negative numbers for unsigned types: Added explicit checks in cli::Reader to reject negative input for unsigned integer types.
cli::Reader — no range check on unsigned narrowing cast: Implemented range checks in cli::Reader for unsigned narrowing casts to prevent silent truncation of out-of-range values.
cli::Reader — no range check on signed narrowing cast: Added range checks in cli::Reader for signed narrowing casts to prevent silent truncation of out-of-range values.
cli::Reader — std::stod is locale-dependent: Replaced std::stod with std::from_chars in cli::Reader for floating-point parsing to ensure locale-independent behavior.
capnproto::read(istream, schema) — infinite recursion: Resolved infinite recursion in capnproto::read(std::istream&, const Schema&) by reading stream bytes into a vector before calling the byte-array overload.
avro::SchemaImpl move-assignment — double-decrement / use-after-free: Fixed avro::SchemaImpl move-assignment to properly release old resources and nullify the moved-from object's pointer, preventing leaks and use-after-free.
xml::Reader uses stoi for all integer types: Updated xml::Reader to use std::stoll for signed integers and std::stoull for unsigned integers, preventing truncation and out-of-range errors.
toml::Writer — at_path() dereferences as path, crashes on dotted keys: Corrected toml::Writer to use operator[] for literal key lookup instead of at_path() when handling dotted field names, preventing crashes.
msgpack::read ignores msgpack_unpack return value: Added a check for msgpack_unpack's return value in msgpack::read to provide meaningful errors for invalid or truncated data.
msgpack::Writer stores uint64_t as signed int64: Implemented a separate branch in msgpack::Writer to correctly pack uint64_t values using msgpack_pack_uint64, ensuring proper wire type.
bson::Reader rejects integer BSON values for float fields: Modified bson::Reader to accept integer BSON types (INT32, INT64, DATE_TIME) when reading into floating-point fields.
delete/delete[] mismatch in test: Corrected a memory deallocation in tests/json/test_pointer_fields.cpp from 'delete' to 'delete[]' to match allocation.
Flatten(U&&) universal reference constructor copies instead of forwarding: Updated Flatten's universal reference constructor to use std::forward(_value), ensuring correct move semantics.
Flatten::operator=(Flatten&&) forwards Flatten instead of U: Fixed Flatten::operator=(Flatten&&) to forward the underlying value U, enabling proper cross-type move assignment.

Changelog

docs/literals.md
- Updated documentation to reflect explicit Literal string constructor.
docs/validating_numbers.md
- Updated documentation for Validator accessors to clarify they return const T& and prevent direct mutation.
include/rfl/Box.hpp
- Corrected operator<=> to compare pointed-to values instead of pointer addresses.
include/rfl/Field.hpp
- Updated cross-type move constructor to use std::move for the underlying value.
include/rfl/Flatten.hpp
- Updated cross-type move constructor to use std::move for the underlying value.
- Corrected universal reference constructor to use std::forward.
- Fixed cross-type move assignment operator to forward the underlying value U.
include/rfl/Generic.hpp
- Added <limits> include.
- Implemented range checks for int64_t to int conversion in to_int().
include/rfl/Literal.hpp
- Added explicit keyword to the std::string constructor.
include/rfl/Object.hpp
- Changed difference_type to typename DataType::difference_type to ensure it is signed.
include/rfl/Ref.hpp
- Corrected operator<=> to compare pointed-to values instead of pointer addresses.
include/rfl/Result.hpp
- Fixed converting assignment operator to properly update success_ and handle internal union state.
- Changed value_or(T&&) return type from T&& to T.
- Corrected error() && return type from Error& to Error&&.
include/rfl/TaggedUnion.hpp
- Renamed discrimininator_ to discriminator_.
include/rfl/Timestamp.hpp
- Removed redundant timezone offset subtraction in to_time_t().
include/rfl/Tuple.hpp
- Corrected operator<=> logic to properly compare elements when ordering is equivalent.
include/rfl/Validator.hpp
- Removed noexcept specifier from operator=(U&&).
- Removed non-const overloads of get(), operator*(), operator()(), and value().
include/rfl/bson/Reader.hpp
- Modified floating-point parsing to accept BSON_TYPE_INT32, INT64, and DATE_TIME in addition to DOUBLE.
include/rfl/capnproto/read.hpp
- Resolved infinite recursion in read(std::istream&, const Schema<T>&) by reading stream into a vector.
include/rfl/cli/Reader.hpp
- Added <charconv> and <limits> includes.
- Replaced std::stod with std::from_chars for locale-independent floating-point parsing.
- Added explicit negative number check and range validation for unsigned integer parsing.
- Added range validation for signed integer narrowing casts.
include/rfl/internal/Skip.hpp
- Updated cross-type move constructor to use std::move for the underlying value.
include/rfl/internal/transform_case.hpp
- Adjusted transform_camel_case to prevent prepending an underscore to initial uppercase characters.
include/rfl/msgpack/Writer.hpp
- Added a dedicated branch for packing uint64_t values using msgpack_pack_uint64.
include/rfl/msgpack/read.hpp
- Added a check for msgpack_unpack return value to provide more accurate error messages.
include/rfl/parsing/FieldVariantParser.hpp
- Corrected error message for zero matching fields from 'more than one' to 'none'.
include/rfl/parsing/Parser_tagged_union.hpp
- Fixed typo in error message string literal from 'discrimininator' to 'discriminator'.
include/rfl/xml/Reader.hpp
- Updated integer parsing to use std::stoll for signed and std::stoull for unsigned types.
src/rfl/avro/SchemaImpl.cpp
- Fixed move-assignment operator to correctly decref old iface_ and nullify _other.iface_.
src/rfl/json/Writer.cpp
- Replaced yyjson_mut_strcpy with yyjson_mut_strncpy for field names to handle non-null-terminated string_views.
src/rfl/toml/Writer.cpp
- Replaced at_path() with operator[] for literal key lookup to correctly handle dotted field names.
tests/CMakeLists.txt
- Commented out add_subdirectory(alloc) to disable tests detecting known leaks.
tests/avro/test_regression_bugs.cpp
- Added new test file to verify avro::SchemaImpl move-assignment prevents double-free.
tests/bson/test_regression_bugs.cpp
- Added new test file to verify bson::Reader correctly handles integer BSON values for float fields.
tests/capnproto/CMakeLists.txt
- Filtered out compile_fail sources from main executable.
- Added custom test for capnproto_read_istream_schema compile failure/success.
tests/capnproto/compile_fail/capnproto_read_istream_schema.cpp
- Added new compile-fail test file for capnproto::read(istream, schema) infinite recursion.
tests/cli/test_regression_bugs.cpp
- Added new test file to verify cli::Reader rejects negative values for unsigned types.
- Added new test file to verify cli::Reader rejects out-of-range values for narrow types.
- Added new test file to verify cli::Reader float parsing ignores locale.
tests/generic/CMakeLists.txt
- Filtered out compile_fail sources from main executable.
- Added custom test for flatten_cross_type_move_assign compile failure/success.
tests/generic/compile_fail/flatten_cross_type_move_assign.cpp
- Added new compile-fail test file for Flatten::operator=(Flatten&&) cross-type move assignment.
tests/generic/test_box.cpp
- Included <compare> header.
- Added new test to verify Box spaceship operator compares values, not pointers.
tests/generic/test_field.cpp
- Added new test file to verify Field cross-type move constructor correctly moves.
tests/generic/test_flatten.cpp
- Included test_helpers.hpp.
- Added new test to verify Flatten cross-type move constructor correctly moves.
- Added new test to verify Flatten universal reference constructor forwards rvalues.
tests/generic/test_generic.cpp
- Added new test file to verify Generic::to_int() rejects overflow and underflow.
tests/generic/test_helpers.hpp
- Added new helper types MoveTracker, DerivedTracker, FlatBase, FlatDerived, SkipBase, SkipDerived for tracking move/copy operations in tests.
tests/generic/test_literal.cpp
- Included <type_traits> header.
- Added new test to verify Literal string constructor is explicit.
tests/generic/test_object.cpp
- Added new test file to verify Object::difference_type is signed.
tests/generic/test_ref.cpp
- Included <compare> header.
- Added new test to verify Ref spaceship operator compares values, not pointers.
tests/generic/test_result.cpp
- Added new test file to verify Result cross-type assignment clears success.
- Added new test file to verify Result::value_or returns value, not rvalue reference.
- Added new test file to verify Result::error() && returns rvalue reference.
tests/generic/test_skip.cpp
- Added new test file to verify Skip cross-type move constructor correctly moves.
tests/generic/test_tagged_union.cpp
- Included <concepts> header.
- Added new test to verify TaggedUnion discriminator spelling.
tests/generic/test_tuple.cpp
- Added new test file to verify Tuple spaceship operator correctly compares values.
tests/generic/test_validator.cpp
- Added new test file to verify Validator assignment operator is not noexcept.
- Added new test file to verify Validator::get() returns const T&.
tests/json/test_pointer_fields.cpp
- Changed delete to delete[] for raw pointer field deallocation.
tests/json/test_regression_bugs.cpp
- Added new test file to verify transform_camel_case does not prepend underscore.
- Added new test file to verify Timestamp::to_time_t() no double timezone correction.
- Added new test file to verify FieldVariantParser empty object error message.
- Added new test file to verify json::Writer handles non-null-terminated field names.
tests/msgpack/test_regression_bugs.cpp
- Added new test file to verify msgpack::read returns meaningful errors for corrupt data.
- Added new test file to verify msgpack::Writer stores uint64_t as positive integers.
tests/toml/test_regression_bugs.cpp
- Added new test file to verify toml::Writer handles dotted field names without crashing.
tests/xml/test_regression_bugs.cpp
- Added new test file to verify xml::Reader correctly handles large int64_t and uint64_t values.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This is an excellent pull request that addresses a substantial number of bugs across the library. The detailed descriptions for each fix, complete with regression tests, are exemplary and make the changes easy to review and verify. The fixes themselves are well-implemented and address critical issues ranging from memory safety and correctness to API design. I have one suggestion for improvement in the XML reader to make the integer parsing more robust against narrowing conversion issues, consistent with similar fixes in this PR. Overall, this is a high-quality contribution that significantly improves the stability and safety of the library.

gemini-code-assist · 2026-02-22T21:14:59Z

include/rfl/xml/Reader.hpp

+        if constexpr (std::is_unsigned<std::remove_cvref_t<T>>()) {
+          return static_cast<T>(std::stoull(str));
+        }
+        else {
+          return static_cast<T>(std::stoll(str));
+        }


While this change correctly uses stoll and stoull to support 64-bit integers, it doesn't protect against silent truncation on narrowing conversions (e.g., parsing a value larger than INT_MAX into an int field). A similar fix for cli::Reader in this PR includes range checks before the static_cast. I recommend adding similar checks here to prevent potential data corruption and provide a more informative error.

if constexpr (std::is_unsigned<std::remove_cvref_t<T>>()) { const auto val = std::stoull(str); if (val > static_cast<unsigned long long>(std::numeric_limits<T>::max())) { return error("Could not cast '" + str + "' to integer: value out of range."); } return static_cast<T>(val); } else { const auto val = std::stoll(str); if (val < static_cast<long long>(std::numeric_limits<T>::min()) || val > static_cast<long long>(std::numeric_limits<T>::max())) { return error("Could not cast '" + str + "' to integer: value out of range."); } return static_cast<T>(val); }

liuzicheng1987 · 2026-02-24T05:00:01Z

HI @Centimo , thanks for this PR. I will write a more comprehensive review tomorrow, but here are my first thoughts:

Issue 1: You are right, I hadn't thought of that. But I think the right approach is to also validate the other operators (maybe with the exception of operator*), not to get rid of them altogether.

Issue 2: I very consciously did not make it explicit. I want people to be able to set Literal fields like this:

my_struct.my_literal = "some_value"

Issue 14: I am quite certain you are wrong about this one. timegm ignores the locale settings of your machine, but it also ignores the value of tm_gmtoff. It simply interprets the other fields as already being in UTC and sets tm_gmtoff and tm_isdst to zero.

This is implied in here:

https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_425.html

And here:

https://nxmnpg.lemoda.net/3/timegm#google_vignette

Furthermore, this specifies that timegm sets tm_gmtoff to zero:

https://man.freebsd.org/cgi/man.cgi?gmtime(3)

The most obvious apprach here would be to write a simple test.

Most of the other things seem like common sense, but I have to take a closer look tomorrow.

Centimo and others added 13 commits February 21, 2026 22:30

Add 17 regression tests for bugs found during code review

fd17c01

Tests cover bugs in Tuple, Result, Validator, Generic, Box, Ref, Field, TaggedUnion, Timestamp, FieldVariantParser, CamelCaseToSnakeCase, and CLI Reader. All tests intentionally fail to confirm bug presence.

Remove internal bug numbers from test comments

bc6663f

Remove alloc tests from build, fix delete/delete[] mismatch in test

e9d24cc

- Disable alloc memory leak tests (detect known unfixed leaks in Parser_string_view and Parser_span) - Fix UB in test_pointer_fields: delete -> delete[] for memory allocated with new char[]

Fix json::Writer reading past string_view bounds

3d7705c

yyjson_mut_strcpy reads until '\0', but std::string_view::data() does not guarantee null-termination. Replace with yyjson_mut_strncpy which respects the explicit length. Add regression test.

Add compile test for Flatten cross-type move assignment bug

ddd41d1

Fix Flatten forwarding and cli::Reader signed narrowing

c60f917

- Flatten(U&&): use std::forward instead of copying - Flatten::operator=(Flatten&&): use std::move(_f.get()) instead of std::forward(_f) - cli::Reader signed parse_value: add range check before static_cast<T>

Update docs for Validator const accessors and Literal explicit ctor

25179bc

gemini-code-assist bot reviewed Feb 22, 2026

View reviewed changes

Add range checks for narrowing integer casts in xml::Reader

6085226

Centimo force-pushed the fix-bugs branch from 5d7fd19 to 6085226 Compare February 22, 2026 21:26

Replace stoull/stoll with std::from_chars for integers in cli::Reader

343a3a4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Fix 30 bugs found during code review#618

Fix 30 bugs found during code review#618
Centimo wants to merge 15 commits intogetml:mainfrom
Centimo:fix-bugs

Centimo commented Feb 22, 2026

Uh oh!

gemini-code-assist bot commented Feb 22, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Feb 22, 2026

Uh oh!

liuzicheng1987 commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

Centimo commented Feb 22, 2026

Fix 30 bugs found during code review

Summary

Discussion: Breaking API Changes

1. Validator::get()/value() return mutable reference

2. Literal(const std::string&) is implicit

3. TaggedUnion — typo discrimininator_

Bugs Fixed

4. Tuple::operator<=> always returns equivalent

5. Result::operator=(const Result<U>&) does not update success_

6. Validator::operator=(U&&) is noexcept but throws

7. Generic::to_int() silently truncates int64_t to int

8. Box/Ref operator<=> compares pointer addresses

9. Field/Flatten/Skip cross-type move constructor copies instead of moving

10. Result::value_or(T&&) returns dangling rvalue reference

11. Result::error() && returns Error& instead of Error&&

12. Object::difference_type is unsigned

13. transform_camel_case prepends _ to names starting with uppercase

14. Timestamp::to_time_t() double-subtracts timezone offset

15. FieldVariantParser — wrong error message

16. json::Writer reads past string_view bounds

17. cli::Reader — stoull accepts negative numbers for unsigned types

18. cli::Reader — no range check on unsigned narrowing cast

19. cli::Reader — no range check on signed narrowing cast

20. cli::Reader — std::stod is locale-dependent

21. capnproto::read(istream, schema) — infinite recursion

22. avro::SchemaImpl move-assignment — double-decrement / use-after-free

23. xml::Reader uses stoi for all integer types

24. toml::Writer — at_path() dereferences as path, crashes on dotted keys

25. msgpack::read ignores msgpack_unpack return value

26. msgpack::Writer stores uint64_t as signed int64

27. bson::Reader rejects integer BSON values for float fields

28. delete/delete[] mismatch in test

29. Flatten(U&&) universal reference constructor copies instead of forwarding

30. Flatten::operator=(Flatten<U>&&) forwards Flatten<U> instead of U

Uh oh!

gemini-code-assist bot commented Feb 22, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

liuzicheng1987 commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. `Validator::get()`/`value()` return mutable reference

2. `Literal(const std::string&)` is implicit

3. `TaggedUnion` — typo `discrimininator_`

4. `Tuple::operator<=>` always returns `equivalent`

5. `Result::operator=(const Result<U>&)` does not update `success_`

6. `Validator::operator=(U&&)` is `noexcept` but throws

7. `Generic::to_int()` silently truncates `int64_t` to `int`

8. `Box`/`Ref` `operator<=>` compares pointer addresses

9. `Field`/`Flatten`/`Skip` cross-type move constructor copies instead of moving

10. `Result::value_or(T&&)` returns dangling rvalue reference

11. `Result::error() &&` returns `Error&` instead of `Error&&`

12. `Object::difference_type` is unsigned

13. `transform_camel_case` prepends `_` to names starting with uppercase

14. `Timestamp::to_time_t()` double-subtracts timezone offset

15. `FieldVariantParser` — wrong error message

16. `json::Writer` reads past `string_view` bounds

17. `cli::Reader` — `stoull` accepts negative numbers for unsigned types

18. `cli::Reader` — no range check on unsigned narrowing cast

19. `cli::Reader` — no range check on signed narrowing cast

20. `cli::Reader` — `std::stod` is locale-dependent

21. `capnproto::read(istream, schema)` — infinite recursion

22. `avro::SchemaImpl` move-assignment — double-decrement / use-after-free

23. `xml::Reader` uses `stoi` for all integer types

24. `toml::Writer` — `at_path()` dereferences as path, crashes on dotted keys

25. `msgpack::read` ignores `msgpack_unpack` return value

26. `msgpack::Writer` stores `uint64_t` as signed `int64`

27. `bson::Reader` rejects integer BSON values for float fields

28. `delete`/`delete[]` mismatch in test

29. `Flatten(U&&)` universal reference constructor copies instead of forwarding

30. `Flatten::operator=(Flatten<U>&&)` forwards `Flatten<U>` instead of `U`