Use bitflags crate for ergonomics#77
Open
The-Nice-One wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR refactors table configuration from individual boolean fields to typed bitflags. ChangesDependency & Core Structure Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/core/tables/character/mod.rs (1)
180-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against panics when modifier flags and record payload diverge.
On Line 184, Line 199, and Line 214,
unwrap()can panic if a flag is set but aCharacterrecord lacks the corresponding value. Prefer returning a serialization error after validating each flagged field is present.Proposed fix
+ for character in &self.characters { + if self.modifier_flags.contains(CharacterTableModifierFlags::UseAdvanceX) + && character.advance_x.is_none() + { + return Err(SerializeError::InvalidTableState); + } + if self.modifier_flags.contains(CharacterTableModifierFlags::UsePixmapIndex) + && character.pixmap_index.is_none() + { + return Err(SerializeError::InvalidTableState); + } + if self + .modifier_flags + .contains(CharacterTableModifierFlags::UsePixmapTableIndex) + && character.pixmap_table_index.is_none() + { + return Err(SerializeError::InvalidTableState); + } + } + for (index, character) in self.characters.iter().enumerate() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tables/character/mod.rs` around lines 180 - 214, The code currently calls unwrap() on character.advance_x, character.pixmap_index, and character.pixmap_table_index when the corresponding modifier_flags (CharacterTableModifierFlags::UseAdvanceX, ::UsePixmapIndex, ::UsePixmapTableIndex) are set, which can panic if the record lacks the value; change each unwrap to validate the Option is Some and, on None, return a serialization error (or map to the existing error type) instead of panicking—locate the blocks that call engine.bytes.push(character.advance_x.unwrap()) / character.pixmap_index.unwrap() / character.pixmap_table_index.unwrap() and replace with safe checks that produce a clear error including which field/table/record failed.src/core/tables/font/serialize.rs (1)
69-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
link_flagsagainstcharacter_table_indexesbefore serialization.If
LinkCharacterTablesis set butcharacter_table_indexesisNone(or vice versa), the encoded link byte and payload structure disagree.Proposed fix
+ if self + .link_flags + .contains(FontTableLinkFlags::LinkCharacterTables) + != self.character_table_indexes.is_some() + { + return Err(SerializeError::InvalidTableState); + } + engine.bytes.push(self.link_flags.bits());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tables/font/serialize.rs` around lines 69 - 84, The serializer writes link_flags.bits() and then unconditionally serializes character_table_indexes, which can produce inconsistent wire format when FontTableLinkFlags::LinkCharacterTables is set but character_table_indexes is None (or vice versa); update the serialization in serialize.rs (the function that pushes engine.bytes and handles character_table_indexes) to validate that self.link_flags.contains(FontTableLinkFlags::LinkCharacterTables) iff self.character_table_indexes.is_some(), return or propagate an error (or panic with a clear message) when the two are inconsistent, and only emit the character_table_indexes payload when the flag is present so the encoded link byte and payload structure always agree (refer to symbols: link_flags, character_table_indexes, FontTableLinkFlags::LinkCharacterTables, engine.bytes.push).src/core/tables/character/serialize.rs (1)
77-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent invalid character-table encoding when flags and optional fields are out of sync.
On Line 77 and Line 131, flags are written from bitfields, but value/link payloads are still emitted from
Optionpresence. Also on Line 85, tagging usesis_some()instead of the emitted flag. These can diverge and produce inconsistent output/tagging.Proposed fix
+ if self + .configuration_flags + .contains(crate::core::CharacterTableConfigurationFlags::ConstantClusterCodePoints) + != self.constant_cluster_codepoints.is_some() + { + return Err(SerializeError::InvalidTableState); + } + if self + .link_flags + .contains(CharacterTableLinkFlags::LinkPixmapTables) + != self.pixmap_table_indexes.is_some() + { + return Err(SerializeError::InvalidTableState); + } + engine.bytes.push(self.configuration_flags.bits()); // Configuration flags byte @@ - value: self.constant_cluster_codepoints.is_some(), + value: self + .configuration_flags + .contains(crate::core::CharacterTableConfigurationFlags::ConstantClusterCodePoints),Also applies to: 131-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tables/character/serialize.rs` around lines 77 - 96, The encoding can become inconsistent because flags are written from self.configuration_flags.bits() while payloads and tagging are gated by Option presence; update serialize logic so payloads (e.g., writing constant_cluster_codepoints) are only emitted when the corresponding flag bit in self.configuration_flags is set (not just Option::is_some()), and update tagging (TagKind::CharacterTableUseConstantClusterCodepoints) and any checks that currently use self.constant_cluster_codepoints.is_some() to instead derive presence from the same flag bit you wrote via self.configuration_flags.bits(); ensure you also use that flag when computing configuration_values_start and when emitting any other optional fields referenced later (the code paths around engine.bytes.push(self.configuration_flags.bits()), the constant_cluster_codepoints emission, and the tagging calls).src/core/tables/pixmap/serialize.rs (1)
54-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce flag-to-payload consistency before writing pixmap table bytes.
configuration_flags.bits()andlink_flags.bits()are now authoritative, but payload emission still depends onOptionfields. If these diverge, the output stream becomes structurally invalid for deserialization.Proposed fix
+ if self + .configuration_flags + .contains(PixmapTableConfigurationFlags::ConstantWidth) + != self.constant_width.is_some() + { + return Err(SerializeError::InvalidTableState); + } + if self + .configuration_flags + .contains(PixmapTableConfigurationFlags::ConstantHeight) + != self.constant_height.is_some() + { + return Err(SerializeError::InvalidTableState); + } + if self + .configuration_flags + .contains(PixmapTableConfigurationFlags::ConstantBitsPerPixel) + != self.constant_bits_per_pixel.is_some() + { + return Err(SerializeError::InvalidTableState); + } + engine.bytes.push(self.configuration_flags.bits()); @@ + if self + .link_flags + .contains(PixmapTableLinkFlags::LinkColorTables) + != self.color_table_indexes.is_some() + { + return Err(SerializeError::InvalidTableState); + } + engine.bytes.push(self.link_flags.bits());Also applies to: 145-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tables/pixmap/serialize.rs` around lines 54 - 119, Before emitting payload bytes, validate that PixmapTableConfigurationFlags (configuration_flags) match the presence of the corresponding Option fields and fail early if they diverge: ensure that if PixmapTableConfigurationFlags::ConstantWidth is set then self.constant_width is Some (and vice versa), same for ConstantHeight and ConstantBitsPerPixel; if a mismatch occurs return an error (or propagate a Serialize error) instead of writing bytes. Update the emit logic around engine.bytes.push (and engine.tags.tag_byte) to only write bytes when the flag is set (not merely when the Option is Some), and apply the same validation for link_flags/payload pairs in the other region (lines ~145-210) so flags are authoritative and serialization cannot produce structurally invalid output.src/core/tables/color/mod.rs (1)
286-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce flag/value invariants before serializing to avoid malformed bytes and panics.
Line 399 writes config flags from
self.configuration_flags, but Line 407/416 behavior is driven byself.constant_alpha. If those diverge, the output layout shifts. Also, Line 290 and Line 303 can panic viaunwrap()when required per-record fields are missing.💡 Suggested fix
pub(crate) fn push_configurations<T: TagWriter>(&self, engine: &mut SerializeEngine<T>) { @@ - engine.bytes.push(self.configuration_flags.bits()); // configuration flags + let use_constant_alpha = self + .configuration_flags + .contains(ColorTableConfigurationFlags::ConstantAlpha); + engine.bytes.push(self.configuration_flags.bits()); @@ - vec![TagKind::ColorTableUseConstantAlpha { + vec![TagKind::ColorTableUseConstantAlpha { table_index: engine.tagging_data.current_table_index, - value: self.constant_alpha.is_some(), + value: use_constant_alpha, }], engine.bytes.byte_index(), ); @@ - if let Some(constant_alpha) = self.constant_alpha { - engine.bytes.push(constant_alpha); + if use_constant_alpha { + let constant_alpha = self + .constant_alpha + .ok_or(SerializeError::InvalidColorTableState)?; + engine.bytes.push(constant_alpha); #[cfg(feature = "tagging")] engine.tags.tag_byte( TagKind::ColorTableConstantAlpha { table_index: engine.tagging_data.current_table_index, value: constant_alpha, }, engine.bytes.byte_index(), ); + } else if self.constant_alpha.is_some() { + return Err(SerializeError::InvalidColorTableState); } }for (index, color) in self.colors.iter().enumerate() { @@ - if self - .modifier_flags - .contains(ColorTableModifierFlags::UseColorType) - { - engine.bytes.push(color.color_type.unwrap() as u8); + if self + .modifier_flags + .contains(ColorTableModifierFlags::UseColorType) + { + let color_type = color + .color_type + .ok_or(SerializeError::InvalidColorTableState)?; + engine.bytes.push(color_type as u8); @@ - if self.constant_alpha.is_none() { - engine.bytes.push(color.custom_alpha.unwrap()); + if !self + .configuration_flags + .contains(ColorTableConfigurationFlags::ConstantAlpha) + { + let custom_alpha = color + .custom_alpha + .ok_or(SerializeError::InvalidColorTableState)?; + engine.bytes.push(custom_alpha);Also applies to: 399-417
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tables/color/mod.rs` around lines 286 - 303, The serializer currently unwraps per-record fields and assumes flag/value invariants (e.g., using ColorTableModifierFlags::UseColorType then color.color_type.unwrap(), and checking self.constant_alpha vs. color.custom_alpha.unwrap()), which can panic or produce malformed layouts; update the serialization in ColorTable (mod.rs) to first validate invariants and handle missing/invalid cases: when self.modifier_flags.contains(ColorTableModifierFlags::UseColorType) only read and push color.color_type after checking color.color_type.is_some() (otherwise return an error or skip with a clear log), and similarly ensure self.constant_alpha.is_none() implies color.custom_alpha.is_some() before unwrapping (otherwise return an explicit error), so no unwrap() is used and the output layout remains consistent when writing bytes via engine.bytes.push and tagging via engine.tags.tag_byte.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/tables/character/mod.rs`:
- Around line 180-214: The code currently calls unwrap() on character.advance_x,
character.pixmap_index, and character.pixmap_table_index when the corresponding
modifier_flags (CharacterTableModifierFlags::UseAdvanceX, ::UsePixmapIndex,
::UsePixmapTableIndex) are set, which can panic if the record lacks the value;
change each unwrap to validate the Option is Some and, on None, return a
serialization error (or map to the existing error type) instead of
panicking—locate the blocks that call
engine.bytes.push(character.advance_x.unwrap()) /
character.pixmap_index.unwrap() / character.pixmap_table_index.unwrap() and
replace with safe checks that produce a clear error including which
field/table/record failed.
In `@src/core/tables/character/serialize.rs`:
- Around line 77-96: The encoding can become inconsistent because flags are
written from self.configuration_flags.bits() while payloads and tagging are
gated by Option presence; update serialize logic so payloads (e.g., writing
constant_cluster_codepoints) are only emitted when the corresponding flag bit in
self.configuration_flags is set (not just Option::is_some()), and update tagging
(TagKind::CharacterTableUseConstantClusterCodepoints) and any checks that
currently use self.constant_cluster_codepoints.is_some() to instead derive
presence from the same flag bit you wrote via self.configuration_flags.bits();
ensure you also use that flag when computing configuration_values_start and when
emitting any other optional fields referenced later (the code paths around
engine.bytes.push(self.configuration_flags.bits()), the
constant_cluster_codepoints emission, and the tagging calls).
In `@src/core/tables/color/mod.rs`:
- Around line 286-303: The serializer currently unwraps per-record fields and
assumes flag/value invariants (e.g., using ColorTableModifierFlags::UseColorType
then color.color_type.unwrap(), and checking self.constant_alpha vs.
color.custom_alpha.unwrap()), which can panic or produce malformed layouts;
update the serialization in ColorTable (mod.rs) to first validate invariants and
handle missing/invalid cases: when
self.modifier_flags.contains(ColorTableModifierFlags::UseColorType) only read
and push color.color_type after checking color.color_type.is_some() (otherwise
return an error or skip with a clear log), and similarly ensure
self.constant_alpha.is_none() implies color.custom_alpha.is_some() before
unwrapping (otherwise return an explicit error), so no unwrap() is used and the
output layout remains consistent when writing bytes via engine.bytes.push and
tagging via engine.tags.tag_byte.
In `@src/core/tables/font/serialize.rs`:
- Around line 69-84: The serializer writes link_flags.bits() and then
unconditionally serializes character_table_indexes, which can produce
inconsistent wire format when FontTableLinkFlags::LinkCharacterTables is set but
character_table_indexes is None (or vice versa); update the serialization in
serialize.rs (the function that pushes engine.bytes and handles
character_table_indexes) to validate that
self.link_flags.contains(FontTableLinkFlags::LinkCharacterTables) iff
self.character_table_indexes.is_some(), return or propagate an error (or panic
with a clear message) when the two are inconsistent, and only emit the
character_table_indexes payload when the flag is present so the encoded link
byte and payload structure always agree (refer to symbols: link_flags,
character_table_indexes, FontTableLinkFlags::LinkCharacterTables,
engine.bytes.push).
In `@src/core/tables/pixmap/serialize.rs`:
- Around line 54-119: Before emitting payload bytes, validate that
PixmapTableConfigurationFlags (configuration_flags) match the presence of the
corresponding Option fields and fail early if they diverge: ensure that if
PixmapTableConfigurationFlags::ConstantWidth is set then self.constant_width is
Some (and vice versa), same for ConstantHeight and ConstantBitsPerPixel; if a
mismatch occurs return an error (or propagate a Serialize error) instead of
writing bytes. Update the emit logic around engine.bytes.push (and
engine.tags.tag_byte) to only write bytes when the flag is set (not merely when
the Option is Some), and apply the same validation for link_flags/payload pairs
in the other region (lines ~145-210) so flags are authoritative and
serialization cannot produce structurally invalid output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b376ff1a-09a4-437b-a701-b42726aed5c8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/core/mod.rssrc/core/tables/character/deserialize.rssrc/core/tables/character/mod.rssrc/core/tables/character/serialize.rssrc/core/tables/color/mod.rssrc/core/tables/font/deserialize.rssrc/core/tables/font/serialize.rssrc/core/tables/pixmap/deserialize.rssrc/core/tables/pixmap/serialize.rs
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
1 participant
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 by CodeRabbit
New Features
Breaking Changes
#[non_exhaustive]markers to public types, restricting exhaustive pattern matching.Chores
serdeto version 1 with explicit feature support and addedbitflags 2.11.1.