Skip to content

Initialize structs using default values before parsing; resolves #719 and #700 - #737

Open
liuzicheng1987 wants to merge 14 commits into
mainfrom
f/init_struct
Open

liuzicheng1987 wants to merge 14 commits into
mainfrom
f/init_struct

Conversation

@liuzicheng1987

Copy link
Copy Markdown
Collaborator

No description provided.

@liuzicheng1987

Copy link
Copy Markdown
Collaborator Author

PR #737 Review — Main Findings

PR: "Initialize structs using default values before parsing" (fixes #719, #700). Replaces the raw-byte-buffer + placement-new + manual-destructor parsing scheme with real value initialization (T t{}) + assignment; makes rfl::Validator store a Result so it can
hold an error state without throwing.

Bugs (verified at PR head, GCC 13.3, repro code in /tmp/rflprobe/)

  1. HIGH — std::terminate (SIGABRT) parsing a map with an invalid Validator key (regression)
    rfl::json::read<std::map<rfl::Validator<int, rfl::Minimum<1>>, std::string>>(R"({"0": "zero"})") → terminate called..., exit 134.
    Root cause: new MapReader::make_key is_validator_v branch rebuilds the key via KeyT(std::move(_p.first)) — the throwing Validator(T&&) constructor (validates, .value() throws) — inside a Result::transform lambda (no exception handling, verified in
    internal/cpp20/Result.hpp), and make_key is const noexcept → escape = terminate. Pre-PR, Validator keys went through the ReflectionType branch whose lambda had try/catch → graceful Result error. Invalid map keys that used to return an error now abort the program.

  2. HIGH — non-default-constructible structs no longer compile (regression)
    read where NDC has only a parameterized constructor → hard error at Parser_default.hpp:692 (T t{}; — "no matching function for NDC::NDC"). The old placement-new approach explicitly supported such types (the comment still above read_struct claims so — now
    stale). Same new hard requirement propagates to named tuples (NamedTuple() = default + nt; in NamedTupleParser::read), arrays (std::array<...> arr{} in ParserArray), and tuples (tup{} in TupleParser). No test covers non-default-constructible targets, so CI won't
    catch it.

  3. HIGH — silent data corruption: const Validator field loses its parsed value
    struct { const rfl::Validator<unsigned, rfl::Minimum<0>> age; const std::string name; } ← {"age": 8, "name": "Maggie"} → parse succeeds with age == 1 (write-back: {"age":1,...}). Verified: the has_const_v branch in read_struct is taken (instrumented), and the
    identical manual steps (read<named_tuple_t> + from_named_tuple) yield age == 8 — so the corruption is inside the branch's pipeline; exact line not yet pinned down. Silent wrong value on the PR's headline new path is the worst failure mode.

Confirmed fixes (PR goals work)

Secondary findings (code-reading, not runtime-verified)

  • Delayed-error landmine (by design, undocumented): a parse can succeed while a Validator field holds an error state (invalid default + field missing under DefaultIfMissing); the first later value()/get()/* call then throws. ParserValidator::write also throws via
    .value() for error-state validators.
  • NamedTuple with const non-pointer fields: no has_const_v escape hatch in NamedTupleParser → likely compile error at the rfl::get(*_view) = ... assignment (and move_to's surviving const_cast branch is now UB if ever reached, since views point at real const
    members).
  • Dead code: internal/is_validator.hpp is now unused (leftover include in ParserBasicType.hpp:18); ParserValidator.hpp drags in unused heavy headers (FieldVariantParser, VariantVariantWrapper, , visit, …) into every TU that includes it.
  • read_struct_with_default lost its try/catch around T t{} — user default constructors that throw (bad_alloc etc.) now escape json::read instead of becoming a Result error.
  • test_const_fields.cpp is fully commented out with a bare // TODO — the const-field feature ships untested (and finding C++ enums are serialized numerically #3 lives on that path).

Bottom line

The core mechanism (real objects instead of placement-new; error-holding Validator) is sound and fixes both target issues, but as-is the PR should not merge: two crash/regression bugs (map-key terminate, non-default-constructible compile break), one silent
corruption bug (const Validator), and the flagship const-field test is disabled. Findings 1–2 are small fixes; finding 3 needs a root-cause pass in the read_struct const branch.

Still open when interrupted: pinning the exact corruption line (3), a backtrace for the terminate (1), and per-file pass over the untouched areas (MapParser write refactor, ArrowReader — its return std::move(*ptr) suggests it may still use the old buffer,
tuple/array const reconstruction, leak tests). Say the word and I'll dispatch the 4 reviewer agents to complete the sweep.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant