Skip to content

Commit

Permalink
Fix unsafe arithmetics and casts
Browse files Browse the repository at this point in the history
  • Loading branch information
pflenker committed Apr 7, 2024
1 parent c234b6e commit 93550f1
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 13 deletions.
15 changes: 11 additions & 4 deletions src/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,14 @@ impl Editor {
}
fn draw_welcome_message() -> Result<(), Error> {
let mut welcome_message = format!("{NAME} editor -- version {VERSION}");
let width = Terminal::size()?.width as usize;
let width = Terminal::size()?.width;
let len = welcome_message.len();
let padding = (width - len) / 2;
let spaces = " ".repeat(padding - 1);
// we allow this since we don't care if our welcome message is put _exactly_ in the middle.
// it's allowed to be a bit to the left or right.
#[allow(clippy::integer_division)]
let padding = (width.saturating_sub(len)) / 2;

let spaces = " ".repeat(padding.saturating_sub(1));
welcome_message = format!("~{spaces}{welcome_message}");
welcome_message.truncate(width);
Terminal::print(welcome_message)?;
Expand All @@ -77,12 +81,15 @@ impl Editor {
let Size { height, .. } = Terminal::size()?;
for current_row in 0..height {
Terminal::clear_line()?;
// we allow this since we don't care if our welcome message is put _exactly_ in the middle.
// it's allowed to be a bit up or down

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

These comments, starting with a //, are aimed at anyone reviewing the internals of this function. They are placed right on top of what they relate to.

#[allow(clippy::integer_division)]
if current_row == height / 3 {
Self::draw_welcome_message()?;
} else {
Self::draw_empty_row()?;
}
if current_row + 1 < height {
if current_row.saturating_add(1) < height {

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

saturating_add doesn't overflow - if it's called on the highest value, it just doesn't add one more.

Terminal::print("\r\n")?;
}
}
Expand Down
37 changes: 29 additions & 8 deletions src/editor/terminal.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,26 @@
use core::fmt::Display;
use crossterm::cursor::{Hide, MoveTo, Show};
use crossterm::style::Print;
use crossterm::terminal::{disable_raw_mode, enable_raw_mode, size, Clear, ClearType};
use crossterm::{queue, Command};
use std::io::{stdout, Error, Write};
use core::fmt::Display;

#[derive(Copy, Clone)]
pub struct Size {
pub height: u16,
pub width: u16,
pub height: usize,
pub width: usize,
}
#[derive(Copy, Clone)]
pub struct Position {
pub x: u16,
pub y: u16,
pub x: usize,
pub y: usize,
}
/// Represents the Terminal.
/// Edge Case for platforms where `usize` < `u16`:
/// Regardless of the actual size of the Terminal, this representation
/// only spans over at most `usize::MAX` or `u16::size` rows/columns, whichever is smaller.
/// Each size returned truncates to min(`usize::MAX`, `u16::MAX`)
/// And should you attempt to set the cursor out of these bounds, it will also be truncated.

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

This comment, started with a ///, are aimed at any developer interacting with the Terminal, without being aware of the internals. Remember: We abstracted crossterm away now, so whoever interacts with Terminal here might not be aware that it exists and uses u16 as its go-to data type.

The /// ensure this comment is visible e.g. in modern Text Editors when people see a Terminal and hover over it.

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

Note that I explain here the big picture implications of the lossy type conversion.

pub struct Terminal;

impl Terminal {
Expand All @@ -38,8 +44,13 @@ impl Terminal {
Self::queue_command(Clear(ClearType::CurrentLine))?;
Ok(())
}
/// Moves the cursor to the given Position.
/// # Arguments
/// * `Position` - the `Position`to move the cursor to. Will be truncated to `u16::MAX` if bigger.
pub fn move_cursor_to(position: Position) -> Result<(), Error> {
Self::queue_command(MoveTo(position.x, position.y))?;
// clippy::as_conversions: See doc above
#[allow(clippy::as_conversions, clippy::cast_possible_truncation)]
Self::queue_command(MoveTo(position.x as u16, position.y as u16))?;
Ok(())
}
pub fn hide_cursor() -> Result<(), Error> {
Expand All @@ -54,16 +65,26 @@ impl Terminal {
Self::queue_command(Print(string))?;
Ok(())
}

/// Returns the current size of this Terminal.
/// Edge Case for systems with `usize` < `u16`:
/// * A `Size` representing the terminal size. Any coordinate `z` truncated to `usize` if `usize` < `z` < `u16`

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

I have put additional comments directly over the functions which behave surprisingly on the edge cases. That's one of the first places anyone would look if this function starts to behave weirdly.

pub fn size() -> Result<Size, Error> {
let (width, height) = size()?;
let (width_u16, height_u16) = size()?;
// clippy::as_conversions: See doc above

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

Even though I left an extensive comment at the top, I refer here again to those docs. Chances are that anyone who comes reviewing this code first looks at the code and its surroundings, and doesn't start with the function doc right away.

#[allow(clippy::as_conversions)]
let height = height_u16 as usize;
// clippy::as_conversions: See doc above

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

I repeat the same again, even though these lines immediately follow one another. Why? Because I can't assume all these lines will always be in vincinity of one another. Someone might rearrange the code, and by repeating the doc I increase the chance that they would also include the comment in their movement.

#[allow(clippy::as_conversions)]
let width = width_u16 as usize;
Ok(Size { height, width })
}
pub fn execute() -> Result<(), Error> {
stdout().flush()?;
Ok(())
}

fn queue_command<T:Command>(command: T) -> Result<(), Error> {
fn queue_command<T: Command>(command: T) -> Result<(), Error> {
queue!(stdout(), command)?;
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![warn(clippy::all, clippy::pedantic, clippy::print_stdout)]
#![warn(clippy::all, clippy::pedantic, clippy::print_stdout, clippy::arithmetic_side_effects, clippy::as_conversions, clippy::integer_division)]

This comment has been minimized.

Copy link
@pflenker

pflenker Apr 7, 2024

Author Owner

Do you think it's lame that I added these clippy warnings here, only to disable them in most places? We'll discuss this in a second.

mod editor;
use editor::Editor;

Expand Down

0 comments on commit 93550f1

Please sign in to comment.