Repository navigation
Conversation
WalkthroughThis refactor introduces ChangesMove ordering refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #445 +/- ##
======================================
+ Coverage 91% 92% +1%
======================================
Files 33 34 +1
Lines 3739 3874 +135
Branches 3739 3874 +135
======================================
+ Hits 3401 3534 +133
Misses 319 319
- Partials 19 21 +2 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cf5def1-9301-4087-8a5f-cc02228d116c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
src/engine/mod.rssrc/main.rssrc/moves/generate/castle.rssrc/moves/generate/leapers.rssrc/moves/generate/mod.rssrc/moves/generate/pawns.rssrc/moves/mod.rssrc/moves/vec.rssrc/position/mod.rssrc/search/mod.rssrc/search/move_evaluator.rssrc/search/perft.rssrc/search/picker.rssrc/search/see.rs
💤 Files with no reviewable changes (1)
- src/search/picker.rs
| fn assert_eq_vec_move(moves: &mut MoveVec, expected: &[&str]) { | ||
| assert_eq!(moves.len(), expected.len()); | ||
| let mov_strs = moves.iter().map(|m| m.to_string()).collect::<Vec<String>>(); | ||
| moves | ||
| let move_vec = moves.into_iter().map(|m| m.to_string()).collect::<Vec<String>>(); | ||
| move_vec | ||
| .iter() | ||
| .map(|m| m.to_string()) | ||
| .for_each(|m| assert!(expected.contains(&m.as_str()), "got: {:?}, expected: {:?}", mov_strs, expected)); | ||
| .for_each(|m| assert!(expected.contains(&m.as_str()), "got: {:?}, expected: {:?}", move_vec, expected)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare the complete move multisets.
The current check accepts duplicates such as actual [a, a] against expected [a, b]. Sort both collections and assert equality.
Proposed fix
fn assert_eq_vec_move(moves: &mut MoveVec, expected: &[&str]) {
- assert_eq!(moves.len(), expected.len());
- let move_vec = moves.into_iter().map(|m| m.to_string()).collect::<Vec<String>>();
- move_vec
- .iter()
- .for_each(|m| assert!(expected.contains(&m.as_str()), "got: {:?}, expected: {:?}", move_vec, expected));
+ let mut actual = moves.into_iter().map(|m| m.to_string()).collect::<Vec<_>>();
+ let mut expected = expected.iter().map(|m| (*m).to_owned()).collect::<Vec<_>>();
+ actual.sort_unstable();
+ expected.sort_unstable();
+ assert_eq!(actual, expected);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn assert_eq_vec_move(moves: &mut MoveVec, expected: &[&str]) { | |
| assert_eq!(moves.len(), expected.len()); | |
| let mov_strs = moves.iter().map(|m| m.to_string()).collect::<Vec<String>>(); | |
| moves | |
| let move_vec = moves.into_iter().map(|m| m.to_string()).collect::<Vec<String>>(); | |
| move_vec | |
| .iter() | |
| .map(|m| m.to_string()) | |
| .for_each(|m| assert!(expected.contains(&m.as_str()), "got: {:?}, expected: {:?}", mov_strs, expected)); | |
| .for_each(|m| assert!(expected.contains(&m.as_str()), "got: {:?}, expected: {:?}", move_vec, expected)); | |
| fn assert_eq_vec_move(moves: &mut MoveVec, expected: &[&str]) { | |
| let mut actual = moves.into_iter().map(|m| m.to_string()).collect::<Vec<_>>(); | |
| let mut expected = expected.iter().map(|m| (*m).to_owned()).collect::<Vec<_>>(); | |
| actual.sort_unstable(); | |
| expected.sort_unstable(); | |
| assert_eq!(actual, expected); | |
| } |
|
|
||
| use crate::moves::Move; | ||
|
|
||
| const MAX_SIZE: usize = 128; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently discard moves after 128 entries.
Legal positions can exceed this capacity, causing search and perft to omit valid branches. Use a proven upper bound and make overflow explicit.
Proposed safeguard
pub fn push_scored(&mut self, mov: Move, score: i8) {
- if self.raw_size < MAX_SIZE {
- self.data[self.raw_size].write((Some(mov), score));
- self.raw_size += 1;
- self.length += 1;
- }
+ assert!(self.raw_size < MAX_SIZE, "MoveVec capacity exceeded");
+ self.data[self.raw_size].write((Some(mov), score));
+ self.raw_size += 1;
+ self.length += 1;
}Also increase MAX_SIZE to a documented bound covering every generated pre-filter move set.
Also applies to: 98-103
|
Against master [hash=64]: 🆗 Elo: -13.90 +/- 17.41, nElo: -17.23 +/- 21.53 |
|
Against v1.6.0 [hash=64]: ✅ Elo: 55.71 +/- 19.08, nElo: 64.03 +/- 21.53 |
Summary by CodeRabbit