Skip to content

docs: thin the commentary back to what the code does not say #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mysma-9403 merged 1 commit into from
Oct 11, 2026
Merged

docs: thin the commentary back to what the code does not say #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions crates/xtce-flight-e2e/tests/against_interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,6 @@ fn a_real_mission_definition_survives_the_interpreter() {

/// A value one bit too wide for its field is refused, not truncated.
///
/// The failure this guards against does not look like a failure: the packet is the right
/// length, every other field is right, and one number is quietly wrong.
#[test]
fn a_value_that_does_not_fit_is_refused() {
use flight_shapes::flight::{Baud, Beacon, EncodeError};
Expand Down Expand Up @@ -311,7 +309,6 @@ fn a_value_that_does_not_fit_is_refused() {
assert!(beacon.encode(&mut buffer).is_ok());
}

/// A buffer that is too small is an error rather than a partial packet.
#[test]
fn a_short_buffer_is_refused() {
use flight_shapes::flight::{Beacon, EncodeError};
Expand Down Expand Up @@ -343,7 +340,6 @@ fn a_short_buffer_is_refused() {
);
}

/// The string rules, which are where an encoder and a decoder most easily disagree.
#[test]
fn string_fields_have_to_fit_exactly_or_be_terminated() {
use flight_shapes::flight::{EncodeError, Mode, StatusReport};
Expand Down Expand Up @@ -405,8 +401,6 @@ fn string_fields_have_to_fit_exactly_or_be_terminated() {

/// Decoded strings and binaries point into the caller's buffer.
///
/// This is the property that makes the generated decoder usable where there is no
/// allocator, so it is worth an assertion rather than a comment.
#[test]
fn decoded_text_borrows_from_the_packet() {
use flight_shapes::flight::StatusReport;
Expand All @@ -429,7 +423,6 @@ fn decoded_text_borrows_from_the_packet() {
}
}

/// Two containers, and the dispatcher picks the one whose criteria hold.
#[test]
fn the_dispatcher_chooses_by_restriction_criteria() {
use flight_shapes::flight::{DecodeError, Packet, decode};
Expand Down Expand Up @@ -536,8 +529,6 @@ fn binary16_narrowing_rounds_to_nearest_even() {
///
/// Encoding is untouched by a calibrator — a spacecraft has an ADC count, and turning counts
/// into engineering units is the ground's job — so what is under test here is the accessor,
/// against `xtce-decode`'s own calibration. Two implementations sharing no code, the same
/// input bits, compared on `to_bits()`.
#[test]
fn calibration_matches_the_interpreter_bit_for_bit() {
let checked = check!(calibrated, "calibrated.xml", None::<&str>, 256u64);
Expand All @@ -555,8 +546,6 @@ fn calibration_matches_the_interpreter_bit_for_bit() {
/// every field, so a criterion on an eight-bit field would hold in one packet in 256: the
/// default branch would be checked thousands of times and the contexts twice, and the test
/// would pass whatever the chain did with them. The fields the criteria test are narrow for
/// that reason, and the counts below fail loudly if a change to the definition, the seed or
/// the harness quietly stops reaching a branch.
#[test]
fn context_calibrators_match_the_interpreter() {
let checked = check!(
Expand Down Expand Up @@ -679,8 +668,6 @@ fn context_calibrators_match_the_interpreter() {
///
/// Two packets, chosen so that reading LATER where the definition says LATER gives the
/// opposite answer on both. The interpreter is asked as well, because agreeing with it is the
/// point — but the assertions on the values stand on their own, so this test still says what
/// it means if the two implementations ever drift together.
#[test]
fn a_criterion_on_a_later_parameter_reads_the_field_being_calibrated() {
use context_calibrated::flight::{Flags, Telemetry};
Expand Down Expand Up @@ -870,7 +857,6 @@ fn a_bounded_spline_refuses_outside_its_points() {
/// check a fixed value either. The check below is that they are nevertheless *there* — a
/// generator that dropped them would move every field after them, and the comparison against
/// the interpreter would catch that; one that dropped a trailing one would not, so the sync
/// marker and the trailer are asserted by hand.
#[test]
fn a_telecommand_survives_the_interpreter() {
let checked = check!(commands, "commands.xml", Some("CmdBaseContainer"), 256u64);
Expand All @@ -883,7 +869,6 @@ fn a_telecommand_survives_the_interpreter() {
/// `SPARE` is four bits that leave `MODE` off a byte boundary; `TRAILER` gives four bytes for
/// a sixteen-bit field, and XTCE does not require those to agree — read as one big-endian
/// number, `DEADBEEF` in sixteen bits is `BEEF`, and an encoder that wrote `DEAD` would
/// produce a packet the ground cannot recognise while its own decoder stayed perfectly happy.
#[test]
fn the_fixed_values_a_command_carries_are_written() {
use commands::flight::{Power, PowerOnContainer, SetGainContainer, SetModeContainer};
Expand Down Expand Up @@ -962,8 +947,6 @@ fn a_container_selected_by_conditions_round_trips() {
/// one to *invert*. Reversing bytes is its own inverse, so a whole-byte field costs nothing
/// — but `SEL` is the interesting one: the criterion compares the value *after* the
/// reversal, so `encode` has to write the reversal undone or the interpreter will not
/// recognise the packet at all. If it got that wrong, every packet here would fail to decode
/// rather than decode to the wrong number.
#[test]
fn little_endian_fields_survive_the_interpreter() {
let checked = check!(byte_order, "byte_order.xml", None::<&str>, 256u64);
Expand All @@ -980,7 +963,6 @@ fn little_endian_fields_survive_the_interpreter() {
///
/// Note the nibble array in the middle. Its elements are four bits, so they are not
/// byte-aligned and neither is anything after them until the array ends — the case where an
/// expansion that quietly rounded to bytes would show up.
#[test]
fn arrays_survive_the_interpreter() {
let checked = check!(arrays, "arrays.xml", None::<&str>, 256u64);
Expand All @@ -993,7 +975,6 @@ fn arrays_survive_the_interpreter() {
/// time this generator runs there are no aggregates, only seventeen fields with names like
/// `STATE.samples[2]`. The fixture nests the two in both directions and ends its aggregate on
/// a four-bit member, so nothing after it is byte-aligned until the pad — an expansion that
/// rounded a member up to a byte would move every field behind it.
#[test]
fn aggregates_survive_the_interpreter() {
let checked = check!(aggregates, "aggregates.xml", None::<&str>, 256u64);
Expand Down
9 changes: 0 additions & 9 deletions crates/xtce-flight/src/emit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,15 +745,6 @@ fn raw_from_value(field: &FlightField, value: &TokenStream) -> TokenStream {
}

/// Puts a little-endian field's bytes the other way round, on the way out.
///
/// The decoder reverses `width / 8` bytes of what it reads, so the encoder reverses them
/// again — which is the same operation, because a byte reversal is its own inverse. Only
/// whole-byte widths reach here; the layout refuses the rest, since reversing a value
/// narrower than its byte count has no inverse at all.
///
/// `swap_bytes` on the widened integer does the reversal, and the shift discards the bytes
/// above the field: a 24-bit value in a `u32` reverses to four bytes, of which the top three
/// are the ones wanted.
fn reverse_if_little_endian(field: &FlightField, raw: &TokenStream) -> TokenStream {
if !field.swap_bytes || field.bit_width <= 8 {
return raw.clone();
Expand Down
23 changes: 0 additions & 23 deletions crates/xtce-flight/tests/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ fn a_plain_container_becomes_one_struct() {
assert!(!container.borrows, "an integer does not borrow");
}

/// A width that is not one of IEEE-754's is refused rather than invented.
#[test]
fn a_float_that_is_not_16_32_or_64_bits_is_refused() {
let message = refusal(layout_of(&one_field(
Expand All @@ -165,7 +164,6 @@ fn a_float_that_is_not_16_32_or_64_bits_is_refused() {
);
}

/// Text has to start on a byte and fill whole ones, because it is written as a slice.
#[test]
fn text_off_a_byte_boundary_is_refused() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -200,7 +198,6 @@ fn text_off_a_byte_boundary_is_refused() {
);
}

/// A width that comes from the packet has no fixed place in a struct.
#[test]
fn a_data_dependent_width_is_refused() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -237,7 +234,6 @@ fn a_data_dependent_width_is_refused() {
);
}

/// A criterion the encoder writes is not also a field the caller sets.
#[test]
fn restriction_criteria_become_constants_and_leave_the_struct() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -290,7 +286,6 @@ fn restriction_criteria_become_constants_and_leave_the_struct() {
);
}

/// An inequality names a set of values, and an encoder has to write one.
#[test]
fn a_non_equality_criterion_is_refused() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -324,7 +319,6 @@ fn a_non_equality_criterion_is_refused() {
);
}

/// Two parameters of the same enumerated type share one generated Rust enum.
#[test]
fn identical_enumerations_are_generated_once() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -355,7 +349,6 @@ fn identical_enumerations_are_generated_once() {
assert_eq!(layout.containers[0].fields[1].kind, Kind::Enumerated(0));
}

/// Labels are free text; identifiers are not.
#[test]
fn labels_that_are_not_identifiers_are_sanitised_without_colliding() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -392,7 +385,6 @@ fn labels_that_are_not_identifiers_are_sanitised_without_colliding() {
assert_eq!(layout.enums[0].variants[1].2, "low power");
}

/// The generated names are the ones a caller types, so they keep the definition's casing.
#[test]
fn type_names_keep_mixed_case_and_fold_shouting() {
use xtce_flight::layout::type_ident;
Expand All @@ -405,7 +397,6 @@ fn type_names_keep_mixed_case_and_fold_shouting() {
assert_eq!(type_ident("---"), "Value");
}

/// An `<ANDedConditions>` is a conjunction, so it becomes constants like any other.
#[test]
fn a_conjunction_of_conditions_becomes_constants() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -467,8 +458,6 @@ fn a_conjunction_of_conditions_becomes_constants() {

/// A disjunction is not a packet.
///
/// A decoder can evaluate any boolean expression — it has the packet in front of it. An
/// encoder has to produce one, and `A == 1 || A == 2` does not say which.
#[test]
fn a_disjunction_of_conditions_is_refused() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -516,8 +505,6 @@ fn a_disjunction_of_conditions_is_refused() {
///
/// A decoder can do it — `xtce-rs` does, because agreeing with the reference is its contract
/// — and a twelve-bit little-endian field then decodes to values up to sixteen bits wide.
/// Most of those cannot be written back into twelve bits at all, so an encoder that accepted
/// the field would produce packets its own decoder disagrees with.
#[test]
fn a_little_endian_field_that_is_not_whole_bytes_is_refused() {
let refused = layout_of(&one_field(
Expand Down Expand Up @@ -548,8 +535,6 @@ fn a_little_endian_field_that_is_not_whole_bytes_is_refused() {
/// A criterion on a little-endian field is written with its bytes the other way round.
///
/// The interpreter compares the value *after* the reversal, so the encoder has to write the
/// reversal undone — and that happens here, when the code is generated, rather than costing
/// anything at run time.
#[test]
fn a_little_endian_criterion_is_inverted_when_it_is_planned() {
let body = r#" <ParameterTypeSet>
Expand Down Expand Up @@ -594,7 +579,6 @@ fn a_little_endian_criterion_is_inverted_when_it_is_planned() {
/// the two are inverses.
///
/// `xtce-rs` decodes it, and matches the Python reference doing so. Reading is well defined;
/// only writing is not.
#[test]
fn a_mil_std_1750a_float_is_refused() {
let message = refusal(layout_of(&one_field(
Expand Down Expand Up @@ -716,8 +700,6 @@ fn a_context_calibrator_resolves_to_the_fields_of_the_struct() {
///
/// Those bits are not a field: they are what makes the packet recognisable, so the encoder
/// writes them and the struct does not carry them. The accessor therefore has nothing to
/// compare, and falling back to the default calibrator would report a number that is wrong
/// in the one way nothing downstream can catch.
#[test]
fn a_context_criterion_on_a_restriction_criterion_is_refused() {
let error = refusal(layout_of(
Expand Down Expand Up @@ -823,7 +805,6 @@ fn a_context_criterion_on_a_wide_boolean_is_refused() {
);
}

/// A `<FixedValueEntry>` becomes bits the encoder writes, reduced to the width it declares.
#[test]
fn a_fixed_value_becomes_bits_the_encoder_writes() {
let plain = layout_of(&one_field(
Expand Down Expand Up @@ -857,8 +838,6 @@ fn a_fixed_value_becomes_bits_the_encoder_writes() {

/// A fixed value wider than a `u64` is written a byte at a time.
///
/// Above 64 bits there is no literal to write it as, but a whole number of bytes starting on
/// one needs no literal: each byte is its own assignment at its own index.
#[test]
fn a_fixed_value_wider_than_64_bits_is_written_byte_by_byte() {
let layout = command_layout(
Expand All @@ -881,8 +860,6 @@ fn a_fixed_value_wider_than_64_bits_is_written_byte_by_byte() {
}

/// One wider than a `u64` that is *not* whole bytes is refused.
///
/// It would have to be shifted across every byte it touches, and nothing in reach asks for it.
#[test]
fn a_wide_fixed_value_off_a_byte_boundary_is_refused() {
let error = refusal(command_layout(
Expand Down
Loading