-
Notifications
You must be signed in to change notification settings - Fork 38
[v4] More CI #46
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
base: modules
Are you sure you want to change the base?
[v4] More CI #46
Conversation
add catch2 to CI overall structure
tmp add nm tmp --remove if no work Revert "tmp --remove if no work" This reverts commit d643bf3. dev flow move to dev presets dont ignore rm ci workflows spell add required add required more flags for debug
f80f04a to
76bffbc
Compare
b023ed8 to
0d6b3bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the libfork library to use a modular architecture with C++23 modules. It removes the old monolithic module structure and introduces a new libfork.core module with a promise implementation. The PR also updates the test infrastructure to use Catch2, enhances the CI/CD pipeline with multiple build configurations, and improves the CMake toolchain files.
Changes:
- Replaced the monolithic
libforkmodule with a modularlibfork.coremodule structure containing promise types and concepts - Migrated from a simple main.cpp test to a Catch2-based test framework with promise tests
- Updated CI workflows to test multiple build configurations (debug-hardened, release, no-except-rtti) and split the workflow steps for better clarity
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/src/promise.cpp | New test file for promise functionality using Catch2 framework |
| test/src/main.cpp | Removed old main.cpp test file |
| test/CMakeLists.txt | Updated to use Catch2 test discovery and glob test sources |
| src/libfork.cxx | Removed old monolithic module interface |
| src/libfork.cpp | Removed old module implementation |
| src/core/promise.cxx | New module partition containing promise types, concepts, and static assertions |
| src/core/core.cxx | New core module primary interface that exports the promise partition |
| CMakeLists.txt | Updated module source files to reference new core module structure |
| cmake/llvm-brew-toolchain.cmake | Enhanced with additional LLVM tools (ar, ranlib, nm) and REQUIRED flags |
| cmake/gcc-brew-toolchain.cmake | Enhanced with GCC-specific tools and binutils configuration |
| CMakePresets.json | Refactored to use CI-focused presets with different build configurations |
| CMakeUserPresets.json | New user preset file for local development workflow |
| .gitignore | Removed CMakeUserPresets.json from gitignore to track it in version control |
| .github/workflows/macos.yml | Updated to use matrix strategy with multiple presets and separated workflow steps |
| .github/workflows/linux.yml | Updated to use matrix strategy with multiple presets and separated workflow steps |
| .github/workflows/linear.yml | New workflow to enforce linear git history on pull requests |
| .clangd | Updated compilation database path to match new preset structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This requires that `T` is `void` a reference or a `std::movable` type. | ||
| */ | ||
| template <typename T> | ||
| concept returnable = std::is_void_v<T> || std::movable<T>; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The returnable concept does not correctly implement the documentation's requirement for reference types. The condition std::movable<T> evaluates to false for reference types, and std::is_void_v<T> also evaluates to false for references. To correctly handle references as documented, the concept should be: concept returnable = std::is_void_v<T> || std::is_reference_v<T> || std::movable<T>;
| * This requires that `T` is `void` a reference or a `std::movable` type. | |
| */ | |
| template <typename T> | |
| concept returnable = std::is_void_v<T> || std::movable<T>; | |
| * This requires that `T` is `void`, a reference, or a `std::movable` type. | |
| */ | |
| template <typename T> | |
| concept returnable = std::is_void_v<T> || std::is_reference_v<T> || std::movable<T>; |
| template <typename T> | ||
| concept returnable = std::is_void_v<T> || std::movable<T>; | ||
|
|
||
| template <typename M> | ||
| concept alloc_mixin = std::is_empty_v<M> || requires { M::operator new(); }; | ||
|
|
||
| template <returnable T> | ||
| struct basic_promise; | ||
|
|
||
| template <> | ||
| struct basic_promise<void> { | ||
| basic_promise<void> *parent; | ||
| }; | ||
|
|
||
| template <returnable T> | ||
| struct basic_promise { | ||
| basic_promise<void> base; | ||
| T *return_address; | ||
| }; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The returnable concept, alloc_mixin concept, and basic_promise template structs are not exported from the module, which makes them inaccessible to module consumers. If these are intended to be part of the public API (as suggested by the documentation on returnable), they should be prefixed with the export keyword.
| TEST_CASE("Promise test", "[promise]") { | ||
| lf::promise<int> p; | ||
| REQUIRE(p.test() == "hi"); |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only verifies the placeholder promise struct with its test() method, but does not provide any test coverage for the actual basic_promise implementation, the returnable concept, or the alloc_mixin concept. Consider adding tests for the actual promise implementation and its core functionality.
| export template <typename T> | ||
| struct promise { | ||
| auto test() -> std::string_view { return "hi"; } | ||
| }; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The promise template struct appears to be a placeholder for testing purposes only, containing just a test() method that returns "hi". This conflicts with the basic_promise implementation below which appears to be the actual promise type. Consider removing this placeholder or clarifying its purpose and relationship to basic_promise in the code documentation.
| concept returnable = std::is_void_v<T> || std::movable<T>; | ||
|
|
||
| template <typename M> | ||
| concept alloc_mixin = std::is_empty_v<M> || requires { M::operator new(); }; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alloc_mixin concept checks for M::operator new() which is invalid syntax. The operator new is a static member function and should be checked as requires { M::operator new(std::size_t); } or similar with appropriate parameters. The current syntax will not compile as a valid requires expression.
| concept alloc_mixin = std::is_empty_v<M> || requires { M::operator new(); }; | |
| concept alloc_mixin = std::is_empty_v<M> || requires { M::operator new(std::size_t{}); }; |
| /** | ||
| * @brief A type returnable from libfork's async functions/coroutines. | ||
| * | ||
| * This requires that `T` is `void` a reference or a `std::movable` type. |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error: "a reference" should be "or a reference". The documentation should read "This requires that T is void, a reference, or a std::movable type."
| * This requires that `T` is `void` a reference or a `std::movable` type. | |
| * This requires that `T` is `void`, a reference, or a `std::movable` type. |
Uh oh!
There was an error while loading. Please reload this page.