drive: static_assert that struct Log stays all-float#460
Closed
eugenevinitsky wants to merge 1 commit into
Closed
Conversation
env_binding.h's vec_log iterates Log as a raw float[] via sizeof(Log)/sizeof(float). A field of a different size (double, pointer) or a forgotten count bump after adding a field would silently misbehave. Wire LOG_NUM_FLOAT_FIELDS to the field count and _Static_assert at the struct boundary that sizeof(struct Log) matches LOG_NUM_FLOAT_FIELDS * sizeof(float). Catches forgotten count updates and wider-than-float field additions at compile time. Doesn't catch a same-sized non-float (e.g. int) if the count is also bumped; the warning comment on the struct flags that footgun for contributors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a compile-time guard to keep struct Log compatible with env_binding.h’s raw-float aggregation path.
Changes:
- Defines
LOG_NUM_FLOAT_FIELDSnext tostruct Log. - Adds documentation that every
Logfield must remain afloat. - Adds a
_Static_assertensuringsizeof(struct Log)matches the expected float field count.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
`env_binding.h`'s `vec_log` iterates `struct Log` as a raw `float[]` via `sizeof(Log)/sizeof(float)`. The existing comment "Will break horribly if Log has non-float data" warns about it, but nothing enforces the invariant. Adding a `double` / pointer field or even an unintended `int` could silently type-pun across the aggregation path.
This PR ties a `LOG_NUM_FLOAT_FIELDS` macro to the field count and `_Static_assert`s at the struct boundary that `sizeof(struct Log) == LOG_NUM_FLOAT_FIELDS * sizeof(float)`. Adding a field requires bumping the count; forgetting to bump it, or sneaking in a wider-than-float field, becomes a compile error.
```c
#define LOG_NUM_FLOAT_FIELDS 54
struct Log {
float n;
...
float reward_ade;
};
_Static_assert(
sizeof(struct Log) == LOG_NUM_FLOAT_FIELDS * sizeof(float),
"struct Log size mismatch: a field was added without bumping "
"LOG_NUM_FLOAT_FIELDS, or a non-float field slipped in (would corrupt "
"vec_log's raw-float iteration).");
```
Gap
A same-sized non-float field (e.g. `int`) with the count bumped would still match `sizeof(Log) == N * sizeof(float)` and slip past the assert — the raw-float iteration would then type-pun the int bytes through `float`-typed reads. The comment at the top of the struct calls this out so a contributor is at least warned. A fully type-safe fix would need an X-macro field-registry refactor, which is much more invasive; this PR aims for the cheap, high-coverage defense.
Test plan
🤖 Generated with Claude Code