Support phlex::sources and implicit providers#637
Open
knoepfel wants to merge 3 commits into
Open
Conversation
Contributor
✅ 7 CodeQL alerts resolved compared to main
Review the full CodeQL report for details. |
knoepfel
force-pushed
the
support-source-macro
branch
3 times, most recently
from
7ce86e6 to
415a7c9
Compare
knoepfel
force-pushed
the
support-source-macro
branch
from
415a7c9 to
c4d0b3d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #637 +/- ##
==========================================
- Coverage 83.27% 83.05% -0.22%
==========================================
Files 162 164 +2
Lines 5912 6002 +90
Branches 670 682 +12
==========================================
+ Hits 4923 4985 +62
- Misses 796 824 +28
Partials 193 193
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
knoepfel
force-pushed
the
support-source-macro
branch
from
c4d0b3d to
4875ba8
Compare
knoepfel
force-pushed
the
support-source-macro
branch
from
4875ba8 to
4f4b2af
Compare
Contributor
Clang-Tidy Check ResultsFound 5948 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
marcpaterno
reviewed
Jun 10, 2026
|
|
||
| TEST_CASE("Implicit providers") | ||
| { | ||
| constexpr auto max_events{3u}; |
Member
There was a problem hiding this comment.
Consider change max_events to num_spills or the like.
marcpaterno
reviewed
Jun 10, 2026
| g.execute(); | ||
|
|
||
| CHECK(g.execution_count("passer") == max_events); | ||
| } |
Member
There was a problem hiding this comment.
Consider checking the execution count on the provider itself, in addition to checking that the transform was triggered (by the provider) the right number of times.
marcpaterno
reviewed
Jun 10, 2026
|
|
||
| CHECK_THROWS_WITH( | ||
| g.execute(), | ||
| Catch::Matchers::ContainsSubstring("Node with name 'vertices_source' already exists")); |
Member
There was a problem hiding this comment.
Consider changing the message in the exception to say "Source with name ...".
marcpaterno
reviewed
Jun 10, 2026
| } | ||
|
|
||
| template <std::derived_from<source> Source, typename... Args> | ||
| void source(std::string name, Args&&... args) |
Member
There was a problem hiding this comment.
Because this creates a new thing in the graph on which it is called, consider renaming to add_source.
This probably should be done for the other node types as well, so an issue that addresses this should be created.
marcpaterno
reviewed
Jun 10, 2026
|
|
||
| /// @brief Registers a source (used by the framework to create provider nodes) | ||
| template <std::derived_from<source> Source, typename... Args> | ||
| void source(std::string name, Args&&... args) |
Member
There was a problem hiding this comment.
The same comment as put in graph::source applies here: consider renaming to add_source.
marcpaterno
reviewed
Jun 10, 2026
| source_map const& sources, | ||
| tbb::flow::graph& g) | ||
| { | ||
| index_router::provider_input_ports_t result; |
Member
There was a problem hiding this comment.
Consider renaming result to resulting_input_ports or the like.
marcpaterno
reviewed
Jun 10, 2026
|
|
||
| auto& bundle = bundles[0]; | ||
| auto const& spec = bundle.specification(); | ||
| auto node = std::make_unique<provider_node>(spec.creator(), |
Member
There was a problem hiding this comment.
Consider filing an issue that makes the argument list of the constructor of provider_node shorter by passing in the provider_bundle rather than passing in the innards of the provider_bundle.
marcpaterno
reviewed
Jun 10, 2026
| std::string layer, | ||
| std::string stage); | ||
|
|
||
| std::function<provider_function_t> release_provider_function(); |
Member
There was a problem hiding this comment.
We have accessors for all data members; consider making this class a struct, adjust member names, and removing accessors.
marcpaterno
reviewed
Jun 10, 2026
| #define PHLEX_DETAIL_REGISTER_PLUGIN(token_type, func_name, dll_alias, ...) \ | ||
| extern "C" PHLEX_DETAIL_SELECT_SIGNATURE(token_type, dll_alias, __VA_ARGS__) | ||
|
|
||
| // ================================================================================================ |
Member
There was a problem hiding this comment.
Are there tests for these macros? If not, some should be added.
marcpaterno
requested changes
Jun 10, 2026
marcpaterno
left a comment
marcpaterno
left a comment
Member
There was a problem hiding this comment.
Please see the already-filed comments.
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
3 participants
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.
This PR implements the majority of features discussed here, such as:
phlex::provider_bundleclassphlex::sourcebase classPHLEX_REGISTER_SOURCEmacro andg.source<T>(...)registration