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

Unnecessarily Cloning, structuring #1905

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
5 changes: 2 additions & 3 deletions src/align.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,9 @@ where
encoded.push((*curr, j - i));
if j == end {
return encoded;
} else {
curr = &sequence[j];
i = j;
}
curr = &sequence[j];
i = j;
}
j += 1;
}
Expand Down
2 changes: 1 addition & 1 deletion src/features/line_numbers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ impl<'a> LineNumbersData<'a> {
// file. In the case of merge commits, it may be longer.
self.line_number =
MinusPlus::new(line_numbers[0].0, line_numbers[line_numbers.len() - 1].0);
let hunk_max_line_number = line_numbers.iter().map(|(n, d)| n + d).max().unwrap();
let hunk_max_line_number = line_numbers.iter().map(|(n, d)| n + d).max().unwrap_or_default();
self.hunk_max_line_number_width =
1 + (hunk_max_line_number as f64).log10().floor() as usize;
self.plus_file = plus_file;
Expand Down
24 changes: 9 additions & 15 deletions src/git_config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,21 +156,15 @@ fn parse_config_from_env_var_value(s: &str) -> HashMap<String, String> {
GIT_CONFIG_PARAMETERS_REGEX
.captures_iter(s)
.map(|captures| {
let (i, j) = match (
captures.get(1),
captures.get(2),
captures.get(3),
captures.get(4),
) {
(Some(_), Some(_), None, None) => (1, 2),
(None, None, Some(_), Some(_)) => (3, 4),
_ => (0, 0),
};
if (i, j) == (0, 0) {
("".to_string(), "".to_string())
} else {
(captures[i].to_string(), captures[j].to_string())
}
let key = captures
.get(1)
.or_else(|| captures.get(3))
.map_or("".to_string(), |m| m.as_str().to_string());
let value = captures
.get(2)
.or_else(|| captures.get(4))
.map_or("".to_string(), |m| m.as_str().to_string());
(key, value)
Copy link
Owner

@dandavison dandavison Nov 15, 2024

Choose a reason for hiding this comment

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

Would you mind reverting this change? It loses the "joint distribution" of the data: i.e. it's not as obvious from the new code that, for example, they will either both be 0 or neither be 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, corrected

})
.collect()
}
Expand Down
10 changes: 5 additions & 5 deletions src/handlers/blame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,15 @@ $
pub fn parse_git_blame_line<'a>(line: &'a str, timestamp_format: &str) -> Option<BlameLine<'a>> {
let caps = BLAME_LINE_REGEX.captures(line)?;

let commit = caps.get(1).unwrap().as_str();
let author = caps.get(2).unwrap().as_str();
let timestamp = caps.get(3).unwrap().as_str();
let commit = caps.get(1)?.as_str();
let author = caps.get(2)?.as_str();
let timestamp = caps.get(3)?.as_str();

let time = DateTime::parse_from_str(timestamp, timestamp_format).ok()?;

let line_number = caps.get(4).unwrap().as_str().parse::<usize>().ok()?;
let line_number = caps.get(4)?.as_str().parse::<usize>().ok()?;

let code = caps.get(5).unwrap().as_str();
Copy link
Collaborator

Choose a reason for hiding this comment

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

unwrap() here is actually okay because the regex is know at compile time. If it is changed but not the corresponding code here then it should crash for the developer and not silently fail and return None.
Similarly for the max line numbers above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unwrap() here is actually okay because the regex is know at compile time. If it is changed but not the corresponding code here then it should crash for the developer and not silently fail and return None. Similarly for the max line numbers above.

should we reverting this changes?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think the only bit of the PR that arguably should be retained is not using else after the early return in src/align.rs -- I do slightly prefer that. But beyond that, there are lots of bugs and feature requests to work on if you've got time! And any performance improvements backed by profiling of course would be welcome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could create and tag issues as 🟣 good first issue for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, corrected

let code = caps.get(5)?.as_str();

Some(BlameLine {
commit,
Expand Down
2 changes: 1 addition & 1 deletion src/options/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub fn set_options(
}
opt.navigate = opt.navigate || opt.env.navigate.is_some();
if opt.syntax_theme.is_none() {
opt.syntax_theme.clone_from(&opt.env.bat_theme);
opt.syntax_theme = opt.env.bat_theme.take();
Copy link
Owner

Choose a reason for hiding this comment

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

While there might be an efficiency saving here, it makes the data inconsistent, so I think we should only make changes like that for demonstrated performance wins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, i see, thanks

}

let option_names = cli::Opt::get_argument_and_option_names();
Expand Down
Loading