Make work items void and parameterless making the work queue api simpler#154
Merged
Make work items void and parameterless making the work queue api simpler#154
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the work queue API by restricting work items to parameterless void() callables, removing the previous support for arbitrary parameters and return values. The simplification enables adding optional string descriptions to enqueued work items via a format string API.
Changes:
- Removed template-based
runnable_work_itemclass that supported arbitrary parameters and return values - Updated
enqueue()to accept onlyvoid()callables with optional format string descriptions - Moved work_item implementation from header to separate source files and changed namespace from
work_queue_impltowork_queue_impl - Changed
description()return type fromstd::wstringtom::not_null<m::cwzstring>for efficiency
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test_work_queue.cpp | Updated test cases to use new simplified enqueue API and added test for description feature |
| work_queue_base.h | Updated do_enqueue signature to accept packaged_task and description, removed do_wait_until |
| work_queue_base.cpp | Consolidated work item creation into do_enqueue, removed do_wait_until implementation |
| work_item.h | New file defining work_item class in work_queue_impl namespace |
| work_item.cpp | New file implementing work_item methods moved from header |
| work_queue.h | Removed inline runnable_work_item template class, simplified enqueue API, added description formatting |
| thread_description.h | Added constructor accepting cwzstring for compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
85bc567 to
22040b2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…wasn't in fact exposed in any case
22040b2 to
0f8aa15
Compare
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.
The work queue api supported passing a general list of parameters to a lambda or other callable and also the callable having a return value that at least in theory could be gotten although the api didn't expose the mechanism to get to it.
This revision simplifies the api to be only
void()so that theenqueue()api can be augmented with the ability to enrichen the queued item with a string description which can be composed via the formatting library.Perhaps this is overkill and an m::wsstring or the like should have been the 2nd parameter, It would be nice to have an experimental language feature to have like parameter groups (like the format string and the parameter lists) for vformat() calls together and be able to specify and move them as a group. Maybe the next next language.