First prototypes #46
First prototypes #46
Conversation
WalkthroughAdds a new C++ header Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Executor
participant AsyncTask
Caller->>Executor: submit(task)
Note over Executor: store task, return
Caller->>Executor: run()
Executor->>AsyncTask: start(ctx)
activate AsyncTask
AsyncTask->>AsyncTask: execute function (possibly async)
AsyncTask-->>Executor: status = Running/Completed/Failed
deactivate AsyncTask
Caller->>AsyncTask: get<ResultType>()
Note right of Caller: blocks/returns result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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 details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/cppcore/Concurrent/async.h(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
include/cppcore/Concurrent/async.h
[error] 3-3: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 3-3: expected ';' after top level declarator
(clang-diagnostic-error)
| class Executor { | ||
| public: | ||
| Executor(); | ||
| ~Executor(); | ||
| Executor(const Executor&) = delete; | ||
| Executor& operator=(const Executor&) = delete; | ||
|
|
||
| void run(); | ||
|
|
||
| bool isRunning() const; | ||
|
|
||
| /** | ||
| * @brief Submits a task for asynchronous execution. | ||
| * | ||
| * @param task The task to be executed. | ||
| */ | ||
| template<typename Task> | ||
| void submit(Task&& task); | ||
|
|
||
| /** | ||
| * @brief Shuts down the executor, waiting for all tasks to complete. | ||
| */ | ||
| void shutdown(); | ||
|
|
||
| private: | ||
| }; |
There was a problem hiding this comment.
Provide concrete definitions for Executor’s API.
run(), isRunning(), submit(), and shutdown() are only declared here, and the PR does not supply definitions anywhere else. The first translation unit that calls any of them will fail to link with unresolved externals. Either supply working implementations in this PR (inline or in a new .cpp) or make the class an abstract interface with pure-virtual members until the executor exists. Right now the header exports a non-functional type.
🤖 Prompt for AI Agents
In include/cppcore/Concurrent/async.h around lines 17 to 42, the Executor class
declares run(), isRunning(), submit(), and shutdown() but provides no
definitions, causing link errors; either implement them here (inline) or convert
the class into an abstract interface with pure-virtual methods. To fix: choose
implementation — add private members (std::thread worker_, std::mutex mu_,
std::condition_variable cv_, std::deque<std::function<void()>> queue_,
std::atomic<bool> running_{false}, std::atomic<bool> stopping_{false});
implement run() to set running_=true and start worker_ looping, waiting on cv_
to pop tasks and execute them until stopping_; implement submit(Task&& t) as a
templated inline method that wraps the task into std::function<void()>, pushes
into queue_ under lock, and notifies cv_; implement isRunning() to return
running_.load(); implement shutdown() to set stopping_=true, notify cv_, join
worker_, and set running_=false. Alternatively, if you prefer an abstract type,
mark run/isRunning/submit/shutdown as pure virtual (virtual ... = 0) and remove
any member-data to avoid exporting a non-functional concrete type.
| public: | ||
| /** | ||
| * @brief Constructs an AsyncTask with the given function. | ||
| * | ||
| * @param func The function to be executed asynchronously. | ||
| */ | ||
| template<typename Func> | ||
| explicit AsyncTask(Func&& func) { | ||
| // Implementation here | ||
| } | ||
|
|
||
| /** | ||
| * @brief Starts the asynchronous task. | ||
| */ | ||
| void start(const context_t& ctx = { nullptr }); | ||
|
|
||
| /// @brief | ||
| /// @return | ||
| AsyncStatus status() const; | ||
|
|
||
| /** | ||
| * @brief Waits for the task to complete and retrieves the result. | ||
| * | ||
| * @return The result of the asynchronous task. | ||
| */ | ||
| template<typename ResultType> | ||
| ResultType get(); | ||
|
|
||
| private: | ||
|
|
||
| }; |
There was a problem hiding this comment.
Finish AsyncTask before publishing it.
AsyncTask exposes start, status, and get but provides no storage for the callable, no state tracking, and—most critically—no definitions for start()/status()/get() in this PR. Instantiating or calling these members will not compile (template get() has no body) or will be undefined behaviour. Please wire the constructor to capture the function, add the necessary state/result members, and implement the lifecycle methods (e.g., backing them with std::future/std::packaged_task). Until that’s in place, the class cannot be used.
🤖 Prompt for AI Agents
In include/cppcore/Concurrent/async.h around lines 52 to 82, AsyncTask currently
declares start(), status(), and template get() but lacks storage for the
callable, task state/result, and implementations; this prevents compilation and
causes UB. Fix by adding members to store the callable (type-erased via
std::function or templated member), a std::packaged_task<R()> or
std::promise/std::future pair to hold the result, an atomic AsyncStatus/state
flag and optional std::thread for execution, and implement the constructor to
capture/move the callable; implement start(const context_t&) to launch the
packaged_task on a new thread (or std::async) and set state, implement status()
to read the atomic state and map future validity to AsyncStatus, and implement
template get() to wait on the future and return the result (propagating
exceptions); ensure proper thread join/detach and move/copy semantics to avoid
races and double-starts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
include/cppcore/Concurrent/async.h (2)
39-64: Critical: Missing method definitions (previously reported).The
Executorclass declaresrun(),isRunning(),submit(), andshutdown()but provides no definitions. This will cause linker errors when any of these methods are called. The previous review comment already covers this critical issue in detail.Additional note: When you implement these methods, you'll need to add the necessary includes at the top of the file:
#include <thread> #include <mutex> #include <condition_variable> #include <functional> #include <deque> #include <atomic>
85-100: Critical: Missing method definitions (previously reported).The
start(),status(), andget<ResultType>()methods are declared but not defined. The previous review comment at lines 74-104 already covers this critical issue and provides detailed guidance on implementation.Minor documentation issue: The doxygen comment for
status()at lines 90-91 is incomplete (empty@briefand@returntags).- /// @brief - /// @return + /// @brief Returns the current status of the asynchronous task. + /// @return The task's AsyncStatus (Pending, Running, Completed, or Failed). AsyncStatus status() const;
🧹 Nitpick comments (1)
include/cppcore/Concurrent/async.h (1)
35-37: Consider renaming and improving type safety.The
_tsuffix is typically reserved for C types or POSIX types. In C++ namespaces, considerContextorContextDatainstead. Additionally, thevoid*lacks type safety and provides no indication of ownership or lifetime semantics.Consider these improvements:
- struct context_t { - void *data; + /// @brief Execution context passed to async tasks. + struct Context { + void *data = nullptr; // Or consider std::any for type-safety (C++17+) };Then update usage throughout:
void start(const Context& ctx = {});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/cppcore/Concurrent/async.h(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
include/cppcore/Concurrent/async.h
[error] 25-25: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 25-25: expected ';' after top level declarator
(clang-diagnostic-error)
🔇 Additional comments (3)
include/cppcore/Concurrent/async.h (3)
1-23: LGTM!The MIT license header is properly formatted and complete. The use of
#pragma onceis acceptable for modern compilers.
28-33: LGTM!The
AsyncStatusenum class provides appropriate states for async task lifecycle management. Usingenum classensures type safety.
80-83: Remove or implement the empty constructor body.The constructor template has an empty implementation body with just a placeholder comment. This is misleading because it compiles without error but is completely non-functional—it doesn't capture or store the
funcparameter. This is actually worse than leaving the method unimplemented (which would cause a linker error and fail fast).Either remove the body entirely to get a linker error:
template<typename Func> - explicit AsyncTask(Func&& func) { - // Implementation here - } + explicit AsyncTask(Func&& func);Or add the necessary private members and implement it properly (per the previous detailed review comment at lines 74-104).
Likely an incorrect or invalid review comment.
|
Summary by CodeRabbit