Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new sudoku-solver/ example: a CLI Sudoku solver implemented with DFS/backtracking and a small shell-based test script, and links it from the repository README.
Changes:
- Introduces a C++23 Sudoku solver CLI that parses an 81-character board string, solves it, and prints solutions (optionally limited by
max_solutions). - Adds a
tests.shscript to validate solving, invalid-input rejection, and themax_solutionslimit behavior. - Adds an example-local Makefile and registers the new example in
README.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sudoku-solver/main.cpp | Implements the solver, input parsing, and CLI output (including max_solutions). |
| sudoku-solver/tests.sh | Adds basic runtime checks for correctness, invalid input handling, and solution limiting. |
| sudoku-solver/Makefile | Builds and runs the new example under the repo’s shared compiler settings. |
| README.md | Adds sudoku-solver to the list of examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| limited_output=$(./sudoku-solver "$puzzle" 1) | ||
| if ! echo "$limited_output" | grep -q "Found 1 solution(s)."; then | ||
| echo "Test failed: max_solutions limit did not work" | ||
| exit 1 |
There was a problem hiding this comment.
This status-line assertion uses grep -q with a pattern containing a . at the end; in regex mode that . matches any character, so the check is less strict than intended. Consider using grep -Fq (or otherwise escaping .) so the test requires the exact output.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| size_t value = 0; | ||
| for (char c : s) { | ||
| if (c < '0' || c > '9') { | ||
| return std::nullopt; | ||
| } | ||
| value = value * 10 + static_cast<size_t>(c - '0'); | ||
| } |
There was a problem hiding this comment.
parse_positive_limit accumulates into a size_t without overflow detection, so very large max_solutions inputs can wrap and be treated as a much smaller (but still positive) limit. Consider rejecting values that would overflow size_t (e.g., pre-check value > (max - digit)/10) and returning nullopt on overflow so the CLI reports invalid input.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| size_t max_solutions = 0; // 0 means unlimited | ||
| if (argc == 3) { | ||
| const auto parsed_limit = parse_positive_limit(argv[2]); | ||
| if (!parsed_limit.has_value()) { | ||
| std::cerr << "Error: max_solutions must be a positive integer.\\n"; | ||
| return 1; | ||
| } | ||
| max_solutions = *parsed_limit; | ||
| } | ||
|
|
||
| auto parsed_state = parse_board(argv[1]); | ||
| if (!parsed_state.has_value()) { | ||
| std::cerr << "Error: invalid board input. Expected 81 chars and no row/column/box conflicts.\\n"; | ||
| return 1; | ||
| } | ||
|
|
||
| SudokuState state = *parsed_state; | ||
| std::vector<std::string> solutions; | ||
| solve_dfs(state, solutions, max_solutions); | ||
|
|
||
| if (solutions.empty()) { | ||
| std::cout << "No solutions found.\\n"; | ||
| return 0; | ||
| } | ||
|
|
||
| std::cout << "Found " << solutions.size() << " solution(s).\\n"; |
There was a problem hiding this comment.
When max_solutions is provided, the solver stops searching once it reaches the limit, but the program still prints Found <solutions.size()> solution(s). which can read like the total number of solutions. Consider tracking whether the search was truncated by the limit and adjusting the summary message (e.g., indicate results were limited / search stopped early) to avoid misleading output.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| output=$(./sudoku-solver "$puzzle") | ||
| if ! echo "$output" | grep -q "$expected"; then | ||
| echo "Test failed: expected solution was not found" | ||
| exit 1 |
There was a problem hiding this comment.
grep -q interprets the pattern as a regular expression. Since expected is intended as a literal string match, consider using fixed-string matching (grep -Fq) to avoid regex edge cases and make the assertion strict.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@doctorlai-msrc I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@doctorlai-msrc I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@doctorlai-msrc I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@doctorlai-msrc I've opened a new pull request, #26, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: doctorlai-msrc <92519564+doctorlai-msrc@users.noreply.github.com> Agent-Logs-Url: https://github.com/DoctorLai/cpp-coding-exercise/sessions/0e440f61-3a33-46cf-b097-29fac49845e0
[WIP] [WIP] Address feedback from review on Sudoku implementation
[WIP] [WIP] Address feedback on Sudoku pull request
Co-authored-by: doctorlai-msrc <92519564+doctorlai-msrc@users.noreply.github.com> Agent-Logs-Url: https://github.com/DoctorLai/cpp-coding-exercise/sessions/046cfbae-a5b3-412f-ade9-bee8689aa4db
[WIP] [WIP] Address feedback from review on Sudoku pull request
[WIP] [WIP] Address feedback from review on Sudoku implementation
This reverts commit 04b66fb.
No description provided.