Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new module test introduces an unused structured-binding element that is likely to fail MSVC builds under /WX (warnings-as-errors) as configured by the project’s default settings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR restores Taskflow’s C++ module build on MSVC by correcting module partition exports/includes, updating the module CMake target to inherit the main library’s build settings, and adding regression coverage plus Windows CI to prevent module import/Executor::async failures (e.g., MSVC C3779).
Changes:
- Fix module interface/partition surface (core/utility) by updating exported symbols and ensuring the public Taskflow entry point is included.
- Adjust TFProf observer header helpers to be usable when consumed via modules (MSVC linkage-related diagnostics).
- Add a transitive-import consumer module test and a Windows Debug/Release CI job that builds and runs the module tests.
File summaries
| File | Description |
|---|---|
unittests/modules/test_tf.cppm |
New consumer module exercising async returns, dependent async, subflows, hashing, and profiler output. |
unittests/modules/test_cxx_modules.cpp |
Adds a transitive-import doctest case that imports test_tf and validates module behavior across worker counts. |
unittests/modules/CMakeLists.txt |
Builds the new test module interface via FILE_SET CXX_MODULES. |
taskflow/observer/tfprof.hpp |
Changes profiler helper functions’ linkage to remain available when used through a module. |
modules/tf.utility.cppm |
Removes exports for obsolete utility symbols (all_same, all_same_v). |
modules/tf.core.cppm |
Includes the public Taskflow header and updates exported symbols (Subflow, GraphLike). |
modules/CMakeLists.txt |
Requires CMake 3.28 for module builds, links module target to Taskflow::Taskflow, and installs module sources under include/taskflow/modules. |
.github/workflows/windows.yml |
Adds a Windows CI job to build/run module tests in Debug/Release. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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.
Related to #780.
This PR restores C++ module builds and asynchronous task imports on the current master branch. On MSVC 19.44, importing
tfand callingExecutor::asyncproduces C3779 because the core partition exportsExecutorwithout including its async definitions. The module interfaces also reference symbols that have since been removed or renamed, preventing the module library from building.The core partition includes the public Taskflow entry point, exports
Subflow, and uses the currentGraphLikename. Obsolete exports are removed. The module-node constructors are defined in class so MSVC 19.51 can construct the node variant through importedcomposed_oftemplates; out-of-class definitions result in C2661 on that compiler. The profiler's two header helpers use external inline linkage so their definitions remain available when the profiler is used through a module; MSVC otherwise reports C2129.The module target inherits Taskflow's include paths, thread linkage, compile options, and feature definitions. Its installed sources go under
include/taskflow/modules, and exported module include paths no longer retain the source checkout. CMake 3.28 is required only when module support is enabled.A consumer module regression test exercises captured async return values, runtime and dependent async tasks, subflows, hashing, and profiler output through a further import. A Windows Debug/Release CI job builds and runs the module tests.
Validation
Locally tested on Windows x64 with Visual Studio 2022, MSVC 19.44.35225, and CMake 3.31.6:
import stdand/std:c++latestcompiled and ran successfully.find_package(Taskflow)compiled and ran with MSVC and Ninja, without the source checkout in its exported module include paths.The installed consumer requires Ninja with this CMake version: its Visual Studio generator does not support the necessary BMI-only compilation. The profiler consumer test includes
<chrono>; removing it exposes a separate MSVC chrono-operator import diagnostic. CUDA and other compiler platforms have not been validated locally.The missing
<algorithm>include mentioned in #780 is already present upstream. VS 2026 / MSVC 19.51.36256 passes all 17 module tests in both Debug and Release in the fork Windows workflow. This verifies the C2661 module-composition failure; the original report's internal compiler crash has not been independently reproduced, so this PR does not claim to resolve every symptom in the issue.Thank you for maintaining Taskflow. I would appreciate your feedback on this approach, particularly whether you would prefer the packaging changes in a separate PR.