Skip to content

simplify the ls code#11391

Open
sylvestre wants to merge 10 commits intouutils:mainfrom
sylvestre:simpl_ls
Open

simplify the ls code#11391
sylvestre wants to merge 10 commits intouutils:mainfrom
sylvestre:simpl_ls

Conversation

@sylvestre
Copy link
Contributor

No description provided.

@github-actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/basenc/bounded-memory is now passing!
Congrats! The gnu test tests/dd/no-allocate is now passing!
Congrats! The gnu test tests/unexpand/bounded-memory is now passing!

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 18, 2026

Merging this PR will improve performance by 3.44%

⚡ 1 improved benchmark
✅ 297 untouched benchmarks
⏩ 48 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation ls_recursive_long_all_balanced_tree[(6, 4, 15)] 133.5 ms 129 ms +3.44%

Comparing sylvestre:simpl_ls (abb52b2) with main (ef8e45c)

Open in CodSpeed

Footnotes

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

@sylvestre sylvestre marked this pull request as ready for review March 19, 2026 06:37
@sylvestre sylvestre requested review from cakebaker and drinkcat March 19, 2026 08:30
Comment on lines +585 to +588
let term_str = term.as_deref().unwrap_or_default().to_string_lossy();
let colorterm_str = colorterm.as_deref().unwrap_or_default().to_string_lossy();

if term.is_some() && term_str.is_empty() && colorterm.is_some() && colorterm_str.is_empty() {
Copy link
Contributor

@cakebaker cakebaker Mar 19, 2026

Choose a reason for hiding this comment

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

While the changes to this function are an improvement, I think using a match would be cleaner. Something like:

    match (term, colorterm) {
        (Some(t), Some(c)) if t.is_empty() && c.is_empty() => false,
        (Some(t), _) if !t.is_empty() => term_matches(&t.to_string_lossy()),
        _ => true,
    }

Please note that the semantics of the third line are slightly different than those of the original code. I don't know if that's an issue.


fn calculate_line_len(output_len: usize, item_len: usize, line_ending: LineEnding) -> usize {
output_len + item_len + line_ending.to_string().len()
fn calculate_line_len(output_len: usize, item_len: usize, _line_ending: LineEnding) -> usize {
Copy link
Contributor

@cakebaker cakebaker Mar 19, 2026

Choose a reason for hiding this comment

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

I would remove either the _line_ending param to simplify the function signature. Or the entire function. The function is called in a single place and so the (trivial) calculation could be done there.

Both approaches also allow you to simplify the signature of update_dired_for_item.

canonical.push_str(value);
Cow::Owned(canonical)
if value.len() == 1 && value.as_bytes()[0].is_ascii_digit() {
Cow::Owned(format!("0{value}"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

My hunch is that the manual string construction is faster than format -- but I haven't benchmarked stuff for a while now, maybe format improved...

Not sure how much you care about performance in this case (didn't look at callers)

.unwrap()
.eq("human-readable"))
let opt_si =
opt_block_size.is_some_and(|v| v == "si") || options.get_flag(options::size::SI);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work? opt_block_size == Some("si") (I think so)

Copy link
Collaborator

@drinkcat drinkcat left a comment

Choose a reason for hiding this comment

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

Overall looks good

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