Skip to content

refactor(uucore): avoid unwrap in system_time_to_sec - #14978

Open
xtqqczze wants to merge 1 commit into
uutils:mainfrom
xtqqczze:uucore-time-system-time-to-sec
Open

refactor(uucore): avoid unwrap in system_time_to_sec#14978
xtqqczze wants to merge 1 commit into
uutils:mainfrom
xtqqczze:uucore-time-system-time-to-sec

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@xtqqczze
xtqqczze force-pushed the uucore-time-system-time-to-sec branch 3 times, most recently from 1b34c17 to 4e4c0b7 Compare September 30, 2026 11:42
xtqqczze
xtqqczze commented Sep 30, 2026
View reviewed changes
Comment thread src/uucore/src/lib/features/time.rs Outdated
@xtqqczze
xtqqczze force-pushed the uucore-time-system-time-to-sec branch from 4e4c0b7 to 531c7ae Compare September 30, 2026 12:37
@codspeed

codspeed Bot commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will degrade performance by 1.26%

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 391 untouched benchmarks
⏩ 54 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation five_38_bit_primes 1.8 s 2 s -7.12%
⚡ Simulation three_39_bit_primes 346.9 ms 330.5 ms +4.96%

Tip

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

Comparing xtqqczze:uucore-time-system-time-to-sec (7ff758a) with main (cacdd8d)

Open in CodSpeed

Footnotes

  1. 54 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. ↩

@xtqqczze
refactor(uucore): avoid unwrap in system_time_to_sec
7ff758a 
- fix `clippy::unwrap_used` lint
- fix `clippy::missing_inline_in_public_items` lint
- fix `clippy::must_use_candidate` lint
@xtqqczze
xtqqczze force-pushed the uucore-time-system-time-to-sec branch from 531c7ae to 7ff758a Compare September 30, 2026 12:43
@xtqqczze

xtqqczze commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Benchmark variance tracked by #14921.

@github-actions

github-actions Bot commented Sep 30, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/date/resolution (passes in this run but fails in the 'main' branch)

@cakebaker

cakebaker commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Doesn't #14980 make system_time_to_sec obsolete?

@xtqqczze

xtqqczze commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

It would still have one use:

coreutils/src/uucore/src/lib/features/time.rs

Line 69 in cacdd8d

let (mut secs, mut nsecs) = system_time_to_sec(time);

But I think that can be inlined.

@xtqqczze

xtqqczze commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Doesn't #14980 make system_time_to_sec obsolete?

It's is a public API though.

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

Reviewers

@cakebaker cakebaker cakebaker left review comments
@RenjiSann RenjiSann RenjiSann left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xtqqczze @cakebaker @RenjiSann