Skip to content

fsext: Cleanup; cut down on unnecessary macros - #14619

Open
kallisti5 wants to merge 2 commits into
uutils:mainfrom
kallisti5:fxext-cleanup
Open

kallisti5 wants to merge 2 commits into
uutils:mainfrom
kallisti5:fxext-cleanup

Conversation

@kallisti5

Copy link
Copy Markdown
  • Some platforms like Haiku offer a i64 for statvfs pointers.
  • The pointer-width checks don't catch i64 vs u64
  • Instead of the duplicated code and excessive platform checks, prefer 'as u64' which catches all situations

* Some platforms like Haiku offer a i64 for statvfs
  pointers.
* The pointer-width checks don't catch i64 vs u64
* Instead of the duplicated code and excessive platform
  checks, prefer 'as u64' which catches all situations
@kallisti5

Copy link
Copy Markdown
Author

This one is based on the conversation in #14607 Cuts way, way, way down on duplicate code while preserving platform compatibility. (hopefully!)

  • Unit tests run on Linux ok and code builds. Unable to test on a lot of the "random less known architectures" however.

@xtqqczze

xtqqczze commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

@kallisti5 sorry, I already had fully tested changes to get fsext to compile on Haiku and NetBSD: #14627

@kallisti5

Copy link
Copy Markdown
Author

In the context of fsext... I feel like this one is a little cleaner 😆 It should accomplish the same thing and work across more platforms more universally.

@xtqqczze

Copy link
Copy Markdown
Collaborator

Sorry, but I think we lose some type-safety with these simplifications. I assume the original code was structured that way for a reason.

@kallisti5

kallisti5 commented Sep 16, 2026 •

Copy link
Copy Markdown
Author

Rust is type-safe at compile though, so that doesn't really make sense.
Overall, the look of this code is "everyone was too afraid to mess with other platforms, so they stacked macros"

I'm 100% sure that there was a reason originally for this design choice, but everyone just stacked tech-debt on-top as they added additional platforms to support.

The rest of the Haiku patches look awesome, but I feel like we have a good opportunity here to cleanup some major tech-debt in the process.

@sylvestre absolutely feel free to disagree with me here though. I'm a newbie making contributions to coreutils

@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/csplit/csplit. tests/csplit/csplit is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/df/df-output. tests/df/df-output is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/du/max-depth. tests/du/max-depth is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/fold/fold. tests/fold/fold is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/head/quote-headers. tests/head/quote-headers is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/ls/ls-time. tests/ls/ls-time is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/ls/m-option. tests/ls/m-option is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/just-created. tests/misc/just-created is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/mv/part-hardlink-symlink. tests/mv/part-hardlink-symlink is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/printf/printf. tests/printf/printf is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/ptx/word-regex-loop. tests/ptx/word-regex-loop is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/seq/seq-precision. tests/seq/seq-precision is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/shuf/shuf. tests/shuf/shuf is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/sort/sort-buffer-size. tests/sort/sort-buffer-size is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/split-io-err. tests/split/split-io-err is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/stat/stat-fmt. tests/stat/stat-fmt is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/stat/stat-nanoseconds. tests/stat/stat-nanoseconds is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail/quote-headers. tests/tail/quote-headers is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/test/test. tests/test/test is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/pipe-f (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/df/skip-rootfs is no longer failing!
Congrats! The gnu test tests/id/smack is no longer failing!
Congrats! The gnu test tests/mkdir/smack-root is no longer failing!
Note: The gnu test tests/mv/i-3 is now being skipped but was previously passing.
Note: The gnu test tests/stty/bad-speed is now being skipped but was previously passing.
Note: The gnu test tests/stty/stty is now being skipped but was previously passing.
Note: The gnu test tests/stty/stty-invalid is now being skipped but was previously passing.
Note: The gnu test tests/stty/stty-pairs is now being skipped but was previously passing.
Note: The gnu test tests/stty/stty-row-col is now being skipped but was previously passing.
Note: The gnu test tests/misc/getopt_vs_usage was skipped on 'main' but is now failing.
Note: The gnu test tests/misc/usage_vs_refs was skipped on 'main' but is now failing.
Note: The gnu test tests/rm/rm-readdir-fail was skipped on 'main' but is now failing.

@xtqqczze

Copy link
Copy Markdown
Collaborator

For example, this case would previously have triggered a panic on x86_64-unknown-netbsd, but no longer does after this change:

$ RUSTC_BOOTSTRAP=1 cargo clippy -q -Zbuild-std -p uucore --features fsext --all-targets --target x86_64-unknown-netbsd -- -D clippy::cast_possible_wrap
error: casting `u64` to `i64` may wrap around the value
   --> src/uucore/src/lib/features/fsext/mod.rs:485:9
    |
485 |         self.f_bsize as i64
    |         ^^^^^^^^^^^^^^^^^^^ help: if this is intentional, use `cast_signed()` instead: `self.f_bsize.cast_signed()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.98.0/index.html#cast_possible_wrap

@xtqqczze

Copy link
Copy Markdown
Collaborator

Sorry, but I think we lose some type-safety with these simplifications. I assume the original code was structured that way for a reason.

@kallisti5 Thank you for pushing back on this. I considered your comments and made some simplifications using try_into().unwrap() and into() for the fs feature in 17f1a5d.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Binary size comparison:

Individual binary size comparison VS main (threshold: >=5% AND >=4 KB).

Total size of compared binaries: 153.03 MB (-2.66 MB, -1.71%)

Significant per-binary changes:
  [        2.32 MB ->    1.12 MB  (-1.20 MB, -51.76%)
  test     2.32 MB ->    1.12 MB  (-1.20 MB, -51.76%)

@codspeed

codspeed Bot commented Oct 6, 2026

Copy link
Copy Markdown

Merging this PR will regress 2 benchmarks

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 11 improved benchmarks
❌ 2 regressed benchmarks
✅ 360 untouched benchmarks
⏩ 50 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation sort_dictionary_order[500000] 1.9 s 2.1 s -6.43%
❌ Simulation ls_recursive_deep_tree[(200, 2)] 3.2 ms 3.3 ms -4.09%
⚡ Simulation complex_relative_date 397.5 µs 287 µs +38.49%
⚡ Simulation check_sorted_utf8_locale 492.2 ms 422.9 ms +16.4%
⚡ Simulation sort_numeric_utf8_locale 41.2 ms 36.5 ms +12.61%
⚡ Simulation merge_single_file_utf8_locale 133.3 ms 119.4 ms +11.66%
⚡ Simulation merge_pre_sorted_files_utf8_locale 251.9 ms 238 ms +5.88%
⚡ Simulation merge_pre_sorted_files 252.8 ms 238.9 ms +5.83%
⚡ Simulation sort_case_sensitive[500000] 339.5 ms 325.7 ms +4.22%
⚡ Simulation sort_german_de_locale 297.9 ms 285.8 ms +4.21%
⚡ Simulation du_wide_tree[(5000, 500)] 18.7 ms 18.1 ms +3.57%
⚡ Simulation du_all_wide_tree[(5000, 500)] 38.2 ms 36.9 ms +3.42%
⚡ Simulation sort_spill_to_tmp_files_utf8_locale 340.2 ms 329.1 ms +3.39%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing kallisti5:fxext-cleanup (8a7f8eb) with main (529f011)

Open in CodSpeed

Footnotes

  1. 50 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

This branch has not been deployed

No deployments
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.

3 participants