Add compile-time and runtime checks for reflectable record layout#35
Conversation
…aint (#21) Member offsets are derived by OffsetFromStart (reinterprets the leading bytes of a pointer-to-member as a byte offset) and DEFINE_MEMBER (offsetof(struct REFLECTABLE, R)). Both are only valid for simple, non-inherited, non-polymorphic structs; a record that violates this (e.g. given a virtual function or a base class) computes wrong offsets silently - data corruption, not a compile or runtime error. Add static_assert(!std::is_polymorphic<REFLECTABLE>::value, ...) right after the macro-generated struct definition in include/reflection.h. This is the portable, always-safe guard: unlike std::is_standard_layout, it doesn't depend on whether std::wstring/TimePoint happen to be standard-layout on a given standard library (verified true on libstdc++ here; unverified on libc++/MSVC), so it can't fail to compile for a currently-valid record on any supported platform. Verified directly: a record with a FUNC(virtual ...) declaration - the realistic way a user would introduce a vtable into a REFLECTABLE struct - now fails to compile with a clear message, where previously offsetof would have only emitted a -Winvalid-offsetof warning and silently computed nonsense. Also verified (Linux/GCC/libstdc++, C++11 and C++20) that is_standard_layout does currently hold for wstring, TimePoint, and all four representative test records, matching the issue's measurement - but since that's unverified on libc++ (macOS) and MSVC (Windows) and this environment can't build for those toolchains, the stronger is_standard_layout assert is deferred to a separate, easily-isolated follow-up commit so CI can decide per-platform rather than guessing. Document the constraint in README.md ("Defining records") and with a comment block next to the REFLECTABLE struct definition. Add tests/reflection_test.cc with static_assert-based compile-time checks (plus a mirroring runtime test) that Person/Pet/Company/DatetimeContainer - one representative of each storage class - satisfy !is_polymorphic, to guard against regression. Full suite passes locally on Ubuntu/GCC in both C++11 and C++20 (75/75 tests); this is itself part of the proof the guard doesn't reject any currently-valid record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
Adds static_assert(std::is_standard_layout<REFLECTABLE>::value, ...) immediately after the polymorphic guard, as its own commit so a CI failure on any platform isolates cleanly to this one static_assert. is_standard_layout on REFLECTABLE is only true if std::wstring and TimePoint both happen to be standard-layout on the active standard library, which is implementation-defined. Verified true here on Ubuntu/GCC/libstdc++ in both C++11 and C++20 (matching the issue's own measurement), for all four representative test records (Person, Pet, Company, DatetimeContainer). NOT verified in this environment on libc++ (macOS) or MSVC (Windows) - this sandbox can only build for Linux/GCC. If CI is green on macOS and Windows (both C++11 and C++20) for this commit, #21 is fully closed. If it fails on either, revert just this commit, keeping the polymorphic guard from the prior commit, and note here that the standard-layout assert is blocked pending #25 (moving the text representation off std::wstring). Also adds a mirroring ReflectionTest.ReflectableRecordsAreStandardLayout runtime test alongside the compile-time check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
CI confirms static_assert(std::is_standard_layout<REFLECTABLE>::value, ...) fails to compile on Windows Latest MSVC, in both C++11 and C++20, for every existing test record - std::wstring and/or sqlite_reflection::TimePoint are not standard-layout under MSVC's standard library. It compiled cleanly on Ubuntu/GCC/libstdc++ and macOS/Clang/libc++ (both C++11 and C++20). This confirms the exact risk the issue called out and the reason this guard was committed separately from the polymorphic one: is_standard_layout on a record is implementation-defined (true only if every member type, including wstring/TimePoint, is itself standard-layout), so it cannot be portably enforced today without breaking a real, currently-valid record on a supported compiler. Revert the assert and its mirroring test, leaving a comment recording what was tried and why, and keep only the portable static_assert(!std::is_polymorphic<REFLECTABLE>::value, ...) from the prior commit. #21 is closed by the polymorphic guard; the standard-layout strengthening is deferred pending #25 (moving the text representation off std::wstring). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
|
CI confirmed the exact risk this issue flagged: Reverted that guard in This closes #21 via the polymorphic guard. The stronger Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4e4a67ba5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static_assert(std::is_standard_layout<REFLECTABLE>::value, | ||
| "sqlite-reflection: reflectable records must be standard-layout."); |
There was a problem hiding this comment.
In the Windows Debug configurations in .github/workflows/cmake.yml, MSVC enables iterator debugging, and its std::wstring is not a standard-layout type. Because the existing test records such as Person, Company, and Pet use MEMBER_TEXT, including those headers instantiates this assertion and the Windows CI build fails before tests run, even though those records were previously supported. The comment above already notes this depends on standard-library string layout; keep only the polymorphic guard or gate the stronger check to platforms where it is known to hold.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed by CI right as this comment landed — MSVC C++11 and C++20 both failed on that exact static_assert, for the same reason you describe. Already reverted in 4ebcfd7: removed the is_standard_layout guard and its mirroring test, kept only the polymorphic guard, and left a comment recording what was tried and why. #21 is closed via the polymorphic guard; the stronger check is deferred pending #25.
Generated by Claude Code
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
This PR adds safeguards to ensure reflectable records maintain the simple, standard-layout struct requirements necessary for correct member offset computation. The changes include compile-time static assertions, runtime test coverage, and documentation updates.
Key Changes
Compile-time validation: Added two
static_assertchecks in theREFLECTABLEmacro definition:is_polymorphiccheck (enforced on all platforms) to reject virtual functions and virtual/multiple inheritanceis_standard_layoutcheck (attempted on all platforms, with platform-specific caveats documented) to catch layout violationsRuntime test coverage: Created
tests/reflection_test.ccwith:Documentation: Updated
README.mdwith a new "Layout constraint" section explaining:offsetof/pointer-to-member byte offsetsImplementation Details
https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK