Commit c4cacba
Add compile-time and runtime checks for reflectable record layout (#35)
* Reject polymorphic reflectable records and document the layout constraint (#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
* Attempt the stronger is_standard_layout guard, isolated for CI (#21)
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
* Revert the is_standard_layout guard: fails on MSVC (#21)
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
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 11812e4 commit c4cacba
3 files changed
Lines changed: 91 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
135 | 135 | | |
136 | 136 | | |
137 | 137 | | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
138 | 146 | | |
139 | 147 | | |
140 | 148 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
| 32 | + | |
32 | 33 | | |
33 | 34 | | |
34 | 35 | | |
| |||
153 | 154 | | |
154 | 155 | | |
155 | 156 | | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
156 | 162 | | |
157 | 163 | | |
158 | 164 | | |
| |||
188 | 194 | | |
189 | 195 | | |
190 | 196 | | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
191 | 215 | | |
192 | 216 | | |
193 | 217 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
0 commit comments