-
Notifications
You must be signed in to change notification settings - Fork 158
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
Add property to enable/disable pruning at runtime #590
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #590 +/- ##
==========================================
+ Coverage 57.23% 57.63% +0.40%
==========================================
Files 130 131 +1
Lines 10434 10621 +187
Branches 956 954 -2
==========================================
+ Hits 5971 6120 +149
- Misses 4416 4454 +38
Partials 47 47 ☔ View full report in Codecov by Sentry. |
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.
Thanks for addressing this, I like the idea of having a simple property based API around this.I am not fully understanding the macros around this, is it necessary to have a macro-based API around this?
core/CMakeLists.txt
Outdated
if(ENABLE_PRUNING) | ||
add_compile_definitions(ENABLE_PRUNING) | ||
endif() | ||
add_compile_definitions(ENABLE_PRUNING=$<BOOL:${ENABLE_PRUNING}>) |
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.
What is that doing?
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.
This is exposing the cmake option as a compiler definition to serve as the default of setPruning()
below.
core/src/task.cpp
Outdated
@@ -92,6 +92,7 @@ const ContainerBase* TaskPrivate::stages() const { | |||
|
|||
Task::Task(const std::string& ns, bool introspection, ContainerBase::pointer&& container) | |||
: WrapperBase(new TaskPrivate(this, ns), std::move(container)) { | |||
setPruning(ENABLE_PRUNING); |
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.
Wouldn't it be possible to just have a C++ API with defaulting to true without exposing this via CMake?
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.
I introduced the cmake option before and would like to keep it to define the default.
Rebase and cleanup of #358. Closes #358.