Skip to content

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

Open
kallisti5 wants to merge 1 commit into
uutils:mainfrom
kallisti5:fxext-cleanup
Open

kallisti5 wants to merge 1 commit 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
Contributor

@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
Contributor

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

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)

@xtqqczze

Copy link
Copy Markdown
Contributor

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

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