Skip to content

Allocate a lot less #202

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.

Sign up for GitHub

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

Open
tanculau wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Allocate a lot less #202

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7b06a7b
Replace compute_stlye with a Writer
tanculau Mar 26, 2025
1ad8d71
fmt
tanculau Mar 26, 2025
82fa2c2
Replace loop with insert_str
tanculau Mar 26, 2025
19faeba
Replaced escape_inner_reset_sequences
tanculau Mar 26, 2025
19eec9b
Fix tests
tanculau Mar 26, 2025
0c6f6c3
Cleanup
tanculau Mar 26, 2025
d540f3d
Change Helper Structs to Tuples
tanculau Mar 26, 2025
b217771
Padding without alloc
tanculau Mar 27, 2025
0ae0b78
Enhance formatting tests
tanculau Mar 27, 2025
57c53ea
Enhance formatting tests
tanculau Mar 27, 2025
ecd6355
Cleanup
tanculau Mar 27, 2025
098fa4f
Remove duplicate code
tanculau Mar 27, 2025
34eaf1a
Remove alloc in closest_color_euclidean
tanculau Mar 27, 2025
baf9453
Replace min_by with min_by_key
tanculau Mar 27, 2025
1e04c4d
Add comments
tanculau Mar 27, 2025
962a9ed
Refactor distance calculation in color comparison
tanculau Mar 27, 2025
d5cd420
Add tests to check if fmt and to_str are equal
tanculau Mar 27, 2025
3e7932b
Split up formatting test and added some
tanculau Mar 27, 2025
d2d4d93
Cleanup
tanculau Mar 27, 2025
383397e
Move tests to their modul
tanculau Mar 27, 2025
362f667
Typo
tanculau Mar 27, 2025
2cd3492
Enhance formatting, escape and color_fn tests
tanculau Mar 28, 2025
432ebe7
Remove allocations in Style
tanculau Apr 7, 2025
aae4077
Merge branch 'master' into less-alloc
tanculau Apr 7, 2025
2497bd0
Add doc comments & AnsiColor test
tanculau Apr 7, 2025
a122bea
Reworked to_static_str, Removed Helper structs, moved formatting in i…
tanculau Sep 5, 2025
5270a3c
Merge branch 'master' into less-alloc
tanculau Sep 5, 2025
a561d42
cargo fmt
tanculau Sep 5, 2025
b0778d0
Changed dyn to impl
tanculau Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ features = ["Win32_Foundation", "Win32_System_Console"]
[dev-dependencies]
ansi_term = "0.12"
insta = "1"
itertools = "0.14.0"
rspec = "1"

[lints.rust]
Expand Down
234 changes: 182 additions & 52 deletions src/color.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{borrow::Cow, cmp, env, str::FromStr};
use std::{borrow::Cow, env, str::FromStr};
use Color::{
AnsiColor, Black, Blue, BrightBlack, BrightBlue, BrightCyan, BrightGreen, BrightMagenta,
BrightRed, BrightWhite, BrightYellow, Cyan, Green, Magenta, Red, TrueColor, White, Yellow,
Expand Down Expand Up @@ -32,59 +32,123 @@ fn truecolor_support() -> bool {
truecolor.is_ok_and(|truecolor| truecolor == "truecolor" || truecolor == "24bit")
}

/// A color that cannot be converted to a [`&'static str`](str)
enum NotStaticColor {
AnsiColor(u8),
TrueColor { r: u8, g: u8, b: u8 },
}

#[allow(missing_docs)]
impl Color {
/// Converts the foreground [`Color`] into a [`&'static str`](str)
///
/// # Errors
///
/// If the color is a `TrueColor` or `AnsiColor`, it will return [`NotStaticColor`] as an Error
const fn to_fg_static_str(self) -> Result<&'static str, NotStaticColor> {
match self {
Self::Black => Ok("30"),
Self::Red => Ok("31"),
Self::Green => Ok("32"),
Self::Yellow => Ok("33"),
Self::Blue => Ok("34"),
Self::Magenta => Ok("35"),
Self::Cyan => Ok("36"),
Self::White => Ok("37"),
Self::BrightBlack => Ok("90"),
Self::BrightRed => Ok("91"),
Self::BrightGreen => Ok("92"),
Self::BrightYellow => Ok("93"),
Self::BrightBlue => Ok("94"),
Self::BrightMagenta => Ok("95"),
Self::BrightCyan => Ok("96"),
Self::BrightWhite => Ok("97"),
Self::TrueColor { r, g, b } => Err(NotStaticColor::TrueColor { r, g, b }),
Self::AnsiColor(code) => Err(NotStaticColor::AnsiColor(code)),
}
}

/// Write [`to_fg_str`](Self::to_fg_str) to the given [`Formatter`](core::fmt::Formatter) without allocating
pub(crate) fn to_fg_fmt(self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
match self.to_fg_static_str() {
Ok(s) => f.write_str(s),
Err(NotStaticColor::TrueColor { r, g, b }) if !truecolor_support() => {
Self::TrueColor { r, g, b }
.closest_color_euclidean()
.to_fg_fmt(f)
}
Err(NotStaticColor::TrueColor { r, g, b }) => write!(f, "38;2;{r};{g};{b}"),
Err(NotStaticColor::AnsiColor(code)) => write!(f, "38;5;{code}"),
}
}

#[must_use]
pub fn to_fg_str(&self) -> Cow<'static, str> {
match *self {
Self::Black => "30".into(),
Self::Red => "31".into(),
Self::Green => "32".into(),
Self::Yellow => "33".into(),
Self::Blue => "34".into(),
Self::Magenta => "35".into(),
Self::Cyan => "36".into(),
Self::White => "37".into(),
Self::BrightBlack => "90".into(),
Self::BrightRed => "91".into(),
Self::BrightGreen => "92".into(),
Self::BrightYellow => "93".into(),
Self::BrightBlue => "94".into(),
Self::BrightMagenta => "95".into(),
Self::BrightCyan => "96".into(),
Self::BrightWhite => "97".into(),
Self::TrueColor { .. } if !truecolor_support() => {
self.closest_color_euclidean().to_fg_str()
match self.to_fg_static_str() {
Ok(s) => s.into(),
Err(NotStaticColor::TrueColor { r, g, b }) if !truecolor_support() => {
Self::TrueColor { r, g, b }
.closest_color_euclidean()
.to_fg_str()
}
Err(NotStaticColor::TrueColor { r, g, b }) => format!("38;2;{r};{g};{b}").into(),
Err(NotStaticColor::AnsiColor(code)) => format!("38;5;{code}").into(),
}
}

/// Converts the background [`Color`] into a [`&'static str`](str)
///
/// # Errors
///
/// If the color is a `TrueColor` or `AnsiColor`, it will return [`NotStaticColor`] as an Error
const fn to_bg_static_str(self) -> Result<&'static str, NotStaticColor> {
match self {
Self::Black => Ok("40"),
Self::Red => Ok("41"),
Self::Green => Ok("42"),
Self::Yellow => Ok("43"),
Self::Blue => Ok("44"),
Self::Magenta => Ok("45"),
Self::Cyan => Ok("46"),
Self::White => Ok("47"),
Self::BrightBlack => Ok("100"),
Self::BrightRed => Ok("101"),
Self::BrightGreen => Ok("102"),
Self::BrightYellow => Ok("103"),
Self::BrightBlue => Ok("104"),
Self::BrightMagenta => Ok("105"),
Self::BrightCyan => Ok("106"),
Self::BrightWhite => Ok("107"),
Self::TrueColor { r, g, b } => Err(NotStaticColor::TrueColor { r, g, b }),
Self::AnsiColor(code) => Err(NotStaticColor::AnsiColor(code)),
}
}

/// Write [`to_bg_str`](Self::to_fg_str) to the given [`Formatter`](core::fmt::Formatter) without allocating
pub(crate) fn to_bg_fmt(self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
match self.to_bg_static_str() {
Ok(s) => f.write_str(s),
Err(NotStaticColor::TrueColor { r, g, b }) if !truecolor_support() => {
Self::TrueColor { r, g, b }
.closest_color_euclidean()
.to_fg_fmt(f)
}
Self::AnsiColor(code) => format!("38;5;{code}").into(),
Self::TrueColor { r, g, b } => format!("38;2;{r};{g};{b}").into(),
Err(NotStaticColor::TrueColor { r, g, b }) => write!(f, "48;2;{r};{g};{b}"),
Err(NotStaticColor::AnsiColor(code)) => write!(f, "48;5;{code}"),
}
}

#[must_use]
pub fn to_bg_str(&self) -> Cow<'static, str> {
match *self {
Self::Black => "40".into(),
Self::Red => "41".into(),
Self::Green => "42".into(),
Self::Yellow => "43".into(),
Self::Blue => "44".into(),
Self::Magenta => "45".into(),
Self::Cyan => "46".into(),
Self::White => "47".into(),
Self::BrightBlack => "100".into(),
Self::BrightRed => "101".into(),
Self::BrightGreen => "102".into(),
Self::BrightYellow => "103".into(),
Self::BrightBlue => "104".into(),
Self::BrightMagenta => "105".into(),
Self::BrightCyan => "106".into(),
Self::BrightWhite => "107".into(),
Self::AnsiColor(code) => format!("48;5;{code}").into(),
Self::TrueColor { .. } if !truecolor_support() => {
self.closest_color_euclidean().to_bg_str()
match self.to_bg_static_str() {
Ok(s) => s.into(),
Err(NotStaticColor::TrueColor { r, g, b }) if !truecolor_support() => {
Self::TrueColor { r, g, b }
.closest_color_euclidean()
.to_bg_str()
}
Self::TrueColor { r, g, b } => format!("48;2;{r};{g};{b}").into(),
Err(NotStaticColor::TrueColor { r, g, b }) => format!("48;2;{r};{g};{b}").into(),
Err(NotStaticColor::AnsiColor(code)) => format!("48;5;{code}").into(),
}
}

Expand All @@ -96,7 +160,7 @@ impl Color {
g: g1,
b: b1,
} => {
let colors = vec![
let colors = [
Black,
Red,
Green,
Expand All @@ -118,19 +182,16 @@ impl Color {
.map(|c| (c, c.into_truecolor()));
let distances = colors.map(|(c_original, c)| {
if let TrueColor { r, g, b } = c {
let rd = cmp::max(r, r1) - cmp::min(r, r1);
let gd = cmp::max(g, g1) - cmp::min(g, g1);
let bd = cmp::max(b, b1) - cmp::min(b, b1);
let rd: u32 = rd.into();
let gd: u32 = gd.into();
let bd: u32 = bd.into();
let distance = rd.pow(2) + gd.pow(2) + bd.pow(2);
fn distance(a: u8, b: u8) -> u32 {
u32::from(a.abs_diff(b)).pow(2)
}
let distance = distance(r, r1) + distance(g, g1) + distance(b, b1);
(c_original, distance)
} else {
unimplemented!("{:?} not a TrueColor", c)
}
});
distances.min_by(|(_, d1), (_, d2)| d1.cmp(d2)).unwrap().0
distances.min_by_key(|(_, distance)| *distance).unwrap().0
Copy link
Collaborator

@spenserblack spenserblack Sep 5, 2025

Choose a reason for hiding this comment

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

This is a nice way to clean up the code, but, TBH, I find this unrelated to the intent of this PR (allocating less). Unless this refactor somehow helps with this PR, I'd rather it be in a separate PR. I prefer to keep unrelated changes out of PRs so that they're more reviewable. Let me know if I'm wrong, and if you feel this belongs in this PR.

Copy link
Author

@tanculau tanculau Sep 5, 2025

Choose a reason for hiding this comment

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

Removed the change.
I just added it, since I was already refactoring the code.

}
c => c,
}
Expand Down Expand Up @@ -262,8 +323,77 @@ fn parse_hex(s: &str) -> Option<Color> {

#[cfg(test)]
mod tests {

pub use super::*;

#[test]
/// Test that `fmt` and `to_str` are the same
fn fmt_and_to_str_same() {
Comment on lines +331 to +333
Copy link
Collaborator

@spenserblack spenserblack Sep 5, 2025

Choose a reason for hiding this comment

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

I think this test touches on something interesting. the to_*_fmt and to_*_str copy a lot of the same logic.
You've defined wrappers the implement Display here, but, TBH, I think this is a sign to de-duplicate the logic between the format and string methods. The string methods should call the format methods.

But that's probably for a separate PR. The reason I bring it up is that, if I'm reading this correctly, what this tests is "did we copy and paste the logic between the two functions?" I'm not a fan of that type of test. Rather a test like this:

#[test]
fn are_some() {
    // Do foo and bar return the same value?
    assert_eq!(x.foo(), x.bar());
}

I'd rather have tests like this:

#[test]
fn test_foo() {
    assert_eq!(x.foo(), some_expected_value);
}

#[test]
fn test_bar() {
    assert_eq!(x.bar(), some_expected_value);
}

That might look redundant, but redundancy is OK in tests IMO.

Copy link
Author

@tanculau tanculau Sep 5, 2025

Choose a reason for hiding this comment

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

I reworked the code, so that there should be as little code duplication as possible.
The problem is, that this is a public interface. As such I cannot change it.

The requirement is, that formatting and the to_str method should always have the same output.
But we cannot write a static str, such that we cannot just call formatting from to_str.
We also want to support precision and advanced formatting. So we also cannot call to_str from the formatting code.
So, if I see it correctly, we have duplicate public interfaces. To check, that both interfaces behave the same, we just call them against each other. So in the end, I only need to test one of them to make sure both work.
The test is also there to protect in the future, if a change occurs, that this requirement is not forgotten.

I'd rather have tests like this:
The problem is, i have to duplicate almost each test case. If a change occurs, the chance of forgetting to update the other one is quite high.

Another option would be to just write a doc comment and hope that everyone reads it.
I think the solution right now should be okay.

use core::fmt::Display;
use itertools::Itertools;
use Color::*;

/// Helper structs to call the method
struct FmtFgWrapper(Color);
impl Display for FmtFgWrapper {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
self.0.to_fg_fmt(f)
}
}
struct FmtBgWrapper(Color);
impl Display for FmtBgWrapper {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
self.0.to_bg_fmt(f)
}
}

// Actual test

let colors = [
Black,
Red,
Green,
Yellow,
Blue,
Magenta,
Cyan,
White,
BrightBlack,
BrightRed,
BrightGreen,
BrightYellow,
BrightBlue,
BrightMagenta,
BrightCyan,
BrightWhite,
]
.into_iter()
.chain(
// Iterator over TrueColors
// r g b
// 0 0 0
// 0 0 1
// 0 0 2
// 0 0 3
// 0 1 0
// ..
// 3 3 3
(0..4)
.combinations_with_replacement(3)
.map(|rgb| Color::TrueColor {
r: rgb[0],
g: rgb[1],
b: rgb[2],
}),
)
.chain((0..4).map(Color::AnsiColor));

for color in colors {
assert_eq!(color.to_fg_str(), FmtFgWrapper(color).to_string());
assert_eq!(color.to_bg_str(), FmtBgWrapper(color).to_string());
}
}

mod from_str {
pub use super::*;

Expand Down
Loading