Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s CI/build infrastructure and developer tooling, standardizing on a single build/ directory with CMake presets, switching GoogleTest to FetchContent, enabling the learning_deadlocks test targets by default, and aligning documentation accordingly.
Changes:
- Update CMake configuration/presets (single
build/dir; add ASan/TSan presets) and switch GoogleTest acquisition to FetchContent. - Enable deadlock test suites in
learning_deadlockswhile disabling a subset of in-progress scenarios. - Add clang tooling configs and a CI formatting job; update docs/README/templates to match the new workflow.
Reviewed changes
Copilot reviewed 90 out of 91 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| learning_stl/tests/test_algorithms.cpp | Marks the parallel sort snippet as a “SOLUTION” comment. |
| learning_shared_ptr/tests/test_multi_threaded_patterns.cpp | Removes an unregistered/optional Asio-based test file. |
| learning_shared_ptr/tests/test_asio_basics.cpp | Removes an unregistered/optional Asio-based test file. |
| learning_deadlocks/tests/test_ownership_transfer_deadlocks.cpp | Disables specific deadlock scenarios while enabling the suite overall. |
| learning_deadlocks/tests/test_condition_variable_deadlocks.cpp | Disables specific deadlock scenarios while enabling the suite overall. |
| learning_deadlocks/tests/test_circular_reference_deadlocks.cpp | Disables specific deadlock scenarios while enabling the suite overall. |
| learning_deadlocks/CMakeLists.txt | Registers deadlock test targets with CTest by default. |
| docs/TEACHING_METHOD.md | Clarifies “requires C++20” and updates Socratic customization instructions. |
| docs/LEARNING_PATH.md | Updates module registration counts and deadlocks module status. |
| docs/BUILDING.md | Updates build requirements/paths and explains deadlocks registration. |
| README.md | Updates prerequisites/paths, adds DQ/DR and exercise marker docs, updates module table notes. |
| CMakePresets.json | Lowers stated preset minimum, unifies build output dir, adds ASan/TSan presets. |
| CMakeLists.txt | Uses FetchContent for GoogleTest and sets C++20 standard explicitly. |
| .gitignore | Stops ignoring clang tooling configs; explicitly keeps them tracked. |
| .github/workflows/ci.yml | Updates caching for unified build dir and enables a format-check job. |
| .github/ISSUE_TEMPLATE/feature_request.md | Adjusts wording around C++ standard selection. |
| .github/ISSUE_TEMPLATE/bug_report.md | Updates run path examples to ./build/<module>/.... |
| .github/CONTRIBUTING.md | Updates dependency/install guidance and removes large inlined style/QAR sections. |
| .cursor/rules/learning-module-implementation.mdc | Adds a methodology doc for implementing new learning modules. |
| .clangd | Points clangd compilation database to build/. |
| .clang-tidy | Adds repository clang-tidy configuration. |
| .clang-format | Adds repository clang-format configuration. |
Comments suppressed due to low confidence (1)
.github/workflows/ci.yml:96
- The formatting job builds the list of files from
git diff --name-only, which will include deleted/renamed files. In this PR two.cppfiles were deleted, soxargs clang-formatwill error on missing paths and fail the job. Filter the diff to existing files (e.g.,git diff --name-only --diff-filter=ACMR) and/or skip non-existent paths before invoking clang-format.
- name: Collect changed C/C++ files
id: changed-files
run: |
if [ "${{ github.event_name }}" = "pull_request" ]; then
BASE="origin/${{ github.base_ref }}"
git fetch origin "${{ github.base_ref }}" --depth=1
git diff --name-only "$BASE"...HEAD > changed_files.txt
else
git diff --name-only "${{ github.event.before }}" "${{ github.sha }}" > changed_files.txt
fi
awk '/\.(cpp|cc|cxx|h|hpp)$/ { print }' changed_files.txt > format_targets.txt
if [ -s format_targets.txt ]; then
echo "has_files=true" >> "$GITHUB_OUTPUT"
else
echo "has_files=false" >> "$GITHUB_OUTPUT"
fi
- name: Check formatting
if: steps.changed-files.outputs.has_files == 'true'
run: |
xargs -a format_targets.txt clang-format --dry-run --Werror --style=file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Brief description of what this PR changes and why.
Fixes #(issue number, if applicable)
Type of Change
Changes Made
Testing
How have you tested these changes?
ctest --preset gcc --verbose)Test Output (if applicable)
Code Quality Checklist
Documentation
docs/LEARNING_PATH.md(if adding or changing module registration)Socratic Pattern (if applicable)
For changes involving Q/A/R content:
Additional Notes
Any additional context, screenshots, or information that reviewers should know.
License Agreement