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

Unwind some growth-ring test nesting #109

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Conversation

richardpringle
Copy link
Contributor

Some simple clean ups

@@ -106,7 +106,7 @@ pub struct WalRingId {
}

impl WalRingId {
pub fn empty_id() -> Self {
pub const fn empty_id() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: impl Default

}
};
if let Some(canvas0) = canvas0 {
let start_ops = if last_idx > 0 { &ops[..last_idx] } else { &[] };
Copy link
Collaborator

Choose a reason for hiding this comment

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

🍹 much cleaner, I can see that this code really bugged you LOL

let mut state = state.clone();
let mut state0 = state.clone();
let nticks = sim.get_nticks(&mut state0);
println!("fail = {}, nticks = {}", f, nticks);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
println!("fail = {}, nticks = {}", f, nticks);
println!("fail = {f}, nticks = {nticks}");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When do we get to turn on clippy::pedantic so it will tell me to do this?

println!("fail = {}, nticks = {}", f, nticks);

for i in 0..nticks {
println!("fail = {}, pos = {}", f, i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i here is pos? Lets call this pos instead of i then.

{
);

if let Ok(_) = sim_result {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if let Ok(_) = sim_result {
if sim_result.is_ok() {

Probably the only good time to use is_ok() is this case.

if sims.len() > 1 {
_multi_point_failure(&sims[1..], &state, f + 1)
track_recursion(&sims[1..], &state, f + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I found this inner function call a little hard to read and understand, but I think it's okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, the inner function is better than fn name(args) { _name(args, 1) }... but I agree that it's bad. I'm going to leave it as is for now.

The better solution would be to just call the check method multiple times (I think), as there's only one case with two sims. But 🤷, I didn't fully try to understand what's happening, just tried to match the behaviour.

@richardpringle richardpringle force-pushed the remove-labelled-break branch from 296bf63 to 83ec583 Compare May 31, 2023 20:57
@richardpringle richardpringle force-pushed the remove-labelled-break branch from 83ec583 to 19a51ee Compare May 31, 2023 22:34
@richardpringle richardpringle merged commit f7eb702 into main Jun 1, 2023
@richardpringle richardpringle deleted the remove-labelled-break branch June 1, 2023 02:51
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