Skip to content

tee: merge flush into write for simplicity#12089

Open
oech3 wants to merge 1 commit intouutils:mainfrom
oech3:tee-flush
Open

tee: merge flush into write for simplicity#12089
oech3 wants to merge 1 commit intouutils:mainfrom
oech3:tee-flush

Conversation

@oech3
Copy link
Copy Markdown
Contributor

@oech3 oech3 commented Apr 30, 2026

Then we can avoid let res = (|| { *** })();.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

GNU testsuite comparison:

Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cut/bounded-memory is now passing!
Congrats! The gnu test tests/printf/printf-surprise is now passing!

@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented Apr 30, 2026

I don't see the benefit of this change, it also means we won't be able to extract Writer to uucore::writable.

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 30, 2026

I believe that there is an advantage to do that more than moving this to uucore since

  • posix tee requirement is specialized compared with other utils
  • let res = ... is nightmare when I call write_all at another place

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 30, 2026

I guess we already have same things on comm and unexpand we can move to uucore.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 30, 2026

Merging this PR will degrade performance by 3.13%

⚠️ 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

❌ 1 regressed benchmark
✅ 310 untouched benchmarks
⏩ 46 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation du_max_depth_balanced_tree[(6, 4, 10)] 25.1 ms 25.9 ms -3.13%

Comparing oech3:tee-flush (d756c7a) with main (5316f58)

Open in CodSpeed

Footnotes

  1. 46 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.

@oech3 oech3 marked this pull request as ready for review April 30, 2026 02:49
@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented Apr 30, 2026

I guess we already have same things on comm and unexpand we can move to uucore.

We could use Writer in place of Box<dyn Write> elsewhere to avoid dynamic dispatch.

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 30, 2026

tee does not require fn write anyway. So we don't need those code on tee.rs.
We can recreate this at uucore if needed.

@xtqqczze
Copy link
Copy Markdown
Contributor

It doesn't make sense to replace idiomatic Write implementation just to avoid one instance of let res = (|| { *** })();.

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 30, 2026 via email

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 30, 2026

Also this is the only way to avoid executing 2 match.

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