Skip to content

fix(sort): replace Element::cmp with key-based comparison to ensure total ordering#31

Closed
cohenna wants to merge 1 commit intoDanielT:mainfrom
cohenna:total_order_sort
Closed

fix(sort): replace Element::cmp with key-based comparison to ensure total ordering#31
cohenna wants to merge 1 commit intoDanielT:mainfrom
cohenna:total_order_sort

Conversation

@cohenna
Copy link
Copy Markdown

@cohenna cohenna commented Jan 8, 2026

Rust 1.92's stricter sort checking panics when the comparison function violates total ordering requirements. The Element::cmp implementation had two issues:

  1. PartialEq used pointer comparison while Ord used content comparison
  2. Natural sorting was applied inconsistently, breaking transitivity

Replace the elem_a.cmp(elem_b) call in ElementRaw::sort() with a custom key-based comparison that provides a consistent total order:

  • INDEX sub-element (numeric, for BSW elements)
  • Item name with natural sorting via (base, number, name) tuple
  • Definition ref (for BSW elements without names)
  • First child character data (for reference elements)

Also remove the nested workspace in autosar-data/Cargo.toml to fix "multiple workspace roots" build error.

…otal ordering

Rust 1.92's stricter sort checking panics when the comparison function
violates total ordering requirements. The Element::cmp implementation
had two issues:
1. PartialEq used pointer comparison while Ord used content comparison
2. Natural sorting was applied inconsistently, breaking transitivity

Replace the `elem_a.cmp(elem_b)` call in ElementRaw::sort() with a
custom key-based comparison that provides a consistent total order:
- INDEX sub-element (numeric, for BSW elements)
- Item name with natural sorting via (base, number, name) tuple
- Definition ref (for BSW elements without names)
- First child character data (for reference elements)

Also remove the nested workspace in autosar-data/Cargo.toml to fix
"multiple workspace roots" build error.
@cohenna cohenna marked this pull request as draft January 8, 2026 15:58
@DanielT
Copy link
Copy Markdown
Owner

DanielT commented Jan 8, 2026

Hi, thanks for the pull request.

Some remarks:

  • What is needed to trigger the panic? I just ran cargo test with current Rust, and that doesn't trigger the issue.
    Ideally the code change would be include a test case that shows the failure in the old version and passes with the fix.

  • From what I can see now, your change still doesn't establish a total order of elements, since all of the checks could show that two elements are equal. For completeness you could finally also check either the position in the input vec, or fall back to pointer value comparison.

  • I don't see any Cargo.toml change in this PR

@DanielT
Copy link
Copy Markdown
Owner

DanielT commented Feb 21, 2026

This seems abandoned.
I'm closing it for now, but feel free to reopen if you want to address the points I raised above.

@DanielT DanielT closed this Feb 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants