Skip to content

Fix build on Redox, and add stat to Redox feature#5502

Merged
sylvestre merged 1 commit intouutils:mainfrom
ids1024:redox
Dec 11, 2023
Merged

Fix build on Redox, and add stat to Redox feature#5502
sylvestre merged 1 commit intouutils:mainfrom
ids1024:redox

Conversation

@ids1024
Copy link
Copy Markdown
Contributor

@ids1024 ids1024 commented Nov 6, 2023

No description provided.

@ids1024 ids1024 force-pushed the redox branch 2 times, most recently from 398ca50 to 03ed9d9 Compare November 6, 2023 02:37
Ok(())
}

#[allow(clippy::unnecessary_cast)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know many occurrences of this happen in this function? If not too many, could you please move that into the function itself

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking at the function again, I see there's already a seperate case with an as u32 for FreeBSD / macOS / Android which are using u16. So Redox could be moved to that case. Though just using a cast with this #[allow] is simpler.

I wonder if it would be better to just not use these constants, and just use octal notation here? Their values should be standardized, and I think people may be more familiar with the octal values for Unix-style permissions than with the constant names.


let result = if mode.chars().any(|c| c.is_ascii_digit()) {
parse_numeric(fperm, mode, true)
parse_numeric(fperm as _, mode, true)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment explaining the "as _"

(srwx, pos)
}

#[allow(clippy::unnecessary_cast)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

@sylvestre
Copy link
Copy Markdown
Contributor

LGTM
it would be great to have CI for this (test would be ideal but at least a job to do cross compilation)

@sylvestre sylvestre merged commit 62afd78 into uutils:main Dec 11, 2023
@jackpot51
Copy link
Copy Markdown
Collaborator

I can work on CI soon. Sorry I haven't gotten to it yet.

@sylvestre
Copy link
Copy Markdown
Contributor

no need to be sorry :)

@jackpot51
Copy link
Copy Markdown
Collaborator

WIP: #5642

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