Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: avoid strange round-trip of draw_fn arguments #1922

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Integral-Tech
Copy link

  • Although format! macro is very powerful, it introduces overhead of memory allocation on the heap compared to &str. Therefore, when format! macro is unnecessary, we should avoid using it.

@th1000s
Copy link
Collaborator

th1000s commented Dec 9, 2024

I noticed that this pattern repeats itself:

let (draw_fn, pad, style) = draw::get_draw_function();

draw_fn(pad, style, ...);

The get returns a function/closure and parameters for it, so those are (directly or indirectly) fed back into it. Instead the returned closure should already handle these, then the if pad { .. } else { .. } is only inside the closure, and decoration_ansi_term_style doesn't make this strange round-trip.

Also, type DrawFunction doesn't have to be a dyn FnMut, just dyn Fn. However, the padding in merge_conflict.rs would also get applied to derived_commit_name, but maybe this visually doesn't make a difference.

@Integral-Tech Integral-Tech changed the title refactor: avoid using format! when it is unnecessary refactor: avoid strange round-trip of draw_fn arguments Dec 17, 2024
@Integral-Tech
Copy link
Author

@th1000s I have just submitted a commit to fix the strange round-trip :)

@th1000s
Copy link
Collaborator

th1000s commented Jan 9, 2025

Thanks! Can you split the concat!() fix into a separate commit?

The match decoration_style { ... } can be made shorter by returning (write_boxed*, style). These write functions are just of type fn(...) -> Result<()>, so no boxing is needed. Then the padding-if-else and calling can all happen in one place, not in every match arm (and then padding could be done for every case).

Also, avoid double negatives, e.g. never_pad = false. In fact, add an enum for that, PadText::{Yes, No}, and place it right after the two text arguments which are padded. Then the purpose of this argument is clear, even without inlay hints.

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