From 8f9b4c10e6075c1ec844a39ba642d952de8f421a Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Sun, 11 Oct 2026 13:17:26 +0200 Subject: [PATCH] docs: thin the commentary back to what the code does not say 51 comment lines go: docs over tests that only said the test's name again, and the paragraphs of a private item's doc that argued with an alternative rather than describing the thing. Left alone: anything naming the no-panic gate, a rounding or alignment rule, a spec clause or a recorded trap, and every doc under tests/, where the comment says why a case exists. Nothing public lost a line. 42 tests, fmt and clippy clean. --- .../tests/against_interpreter.rs | 19 --------------- crates/xtce-flight/src/emit.rs | 9 -------- crates/xtce-flight/tests/layout.rs | 23 ------------------- 3 files changed, 51 deletions(-) diff --git a/crates/xtce-flight-e2e/tests/against_interpreter.rs b/crates/xtce-flight-e2e/tests/against_interpreter.rs index 425c6c8..6c72633 100644 --- a/crates/xtce-flight-e2e/tests/against_interpreter.rs +++ b/crates/xtce-flight-e2e/tests/against_interpreter.rs @@ -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}; @@ -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}; @@ -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}; @@ -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; @@ -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}; @@ -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); @@ -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!( @@ -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}; @@ -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); @@ -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}; @@ -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); @@ -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); @@ -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); diff --git a/crates/xtce-flight/src/emit.rs b/crates/xtce-flight/src/emit.rs index 467eff5..bc9094c 100644 --- a/crates/xtce-flight/src/emit.rs +++ b/crates/xtce-flight/src/emit.rs @@ -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(); diff --git a/crates/xtce-flight/tests/layout.rs b/crates/xtce-flight/tests/layout.rs index 34936ad..9c83b9b 100644 --- a/crates/xtce-flight/tests/layout.rs +++ b/crates/xtce-flight/tests/layout.rs @@ -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( @@ -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#" @@ -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#" @@ -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#" @@ -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#" @@ -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#" @@ -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#" @@ -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; @@ -405,7 +397,6 @@ fn type_names_keep_mixed_case_and_fold_shouting() { assert_eq!(type_ident("---"), "Value"); } -/// An `` is a conjunction, so it becomes constants like any other. #[test] fn a_conjunction_of_conditions_becomes_constants() { let body = r#" @@ -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#" @@ -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( @@ -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#" @@ -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( @@ -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( @@ -823,7 +805,6 @@ fn a_context_criterion_on_a_wide_boolean_is_refused() { ); } -/// A `` 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( @@ -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( @@ -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(