Repository: alacritty/alacritty
File: alacritty_terminal/src/term/color.rs (Rust)

Code before:
    }
}

impl FromStr for Rgb {
    type Err = ();

    fn from_str(s: &str) -> std::result::Result<Rgb, ()> {
        let mut chars = s.chars();
        let mut rgb = Rgb::default();

        macro_rules! component {
            ($($c:ident),*) => {
                $(
                    match chars.next().and_then(|c| c.to_digit(16)) {
                        Some(val) => rgb.$c = (val as u8) << 4,
                        None => return Err(())
                    }

                    match chars.next().and_then(|c| c.to_digit(16)) {
                        Some(val) => rgb.$c |= val as u8,
                        None => return Err(())
                    }
                )*
            }
        }

        match chars.next() {
            Some('0') => {
                if chars.next() != Some('x') {
                    return Err(());
                }
            },
            Some('#') => (),
            _ => return Err(()),
        }

        component!(r, g, b);

        if chars.as_str().is_empty() {
            Ok(rgb)
        } else {
            // We still have something to parse, so throw an error
            Err(())
        }
    }
}

/// List of indexed colors
///
/// The first 16 entries are the standard ansi named colors. Items 16..232 are
/// the color cube.  Items 233..256 are the grayscale ramp. Item 256 is

Review comment (refactor):
I wonder if it wouldn't be much easier to check the length before starting the parsing.

That way we can just skip a lot of things.

I was thinking something like this, wrt method structure:

```rust
let chars = if input.starts_with("0x") && input.len() == 8 {
} else if input.starts_with("#") && input.len() == 7 {
} else {
     return Err(());
};
```

Then we can potentially even clean up the macro a bit, or remove it completely. We can also change it from a chars iterator to a slice reference, by returning something like `&input[1..]`, because that way we can directly use `slice[1]` to index it, instead of having to use `chars.next()`.

Just some ideas though, I think it would be possible to simplify and shorten things up a bit.

Code after:
    }
}

impl FromStr for Rgb {
    type Err = ();

    fn from_str(s: &str) -> std::result::Result<Rgb, ()> {
        let chars = if s.starts_with("0x") && s.len() == 8 {
            &s[2..]
        } else if s.starts_with('#') && s.len() == 7 {
            &s[1..]
        } else {
            return Err(());
        };

        match u32::from_str_radix(chars, 16) {
            Ok(mut color) => {
                let b = (color & 0xff) as u8;
                color >>= 8;
                let g = (color & 0xff) as u8;
                color >>= 8;
                let r = color as u8;
                Ok(Rgb { r, g, b })
            },
            Err(_) => Err(()),
        }
    }
}

/// List of indexed colors
///
/// The first 16 entries are the standard ansi named colors. Items 16..232 are
/// the color cube.  Items 233..256 are the grayscale ramp. Item 256 is
/// the configured foreground color, item 257 is the configured background
/// color, item 258 is the cursor color. Following that are 8 positions for dim colors.
/// Item 267 is the bright foreground color, 268 the dim foreground.
#[derive(Copy, Clone)]
pub struct List([Rgb; COUNT]);

impl<'a> From<&'a Colors> for List {
    fn from(colors: &Colors) -> List {
        // Type inference fails without this annotation
        let mut list = List([Rgb::default(); COUNT]);

        list.fill_named(colors);
        list.fill_cube(colors);
        list.fill_gray_ramp(colors);

        list
    }
}