szaszm commented on a change in pull request #735: MINIFICPP-1158 - Event
driven processors can starve each other
URL: https://github.com/apache/nifi-minifi-cpp/pull/735#discussion_r385330893
##########
File path: libminifi/include/SchedulingAgent.h
##########
@@ -202,7 +134,7 @@ class SchedulingAgent {
std::shared_ptr<core::ContentRepository> content_repo_;
// thread pool for components.
- utils::ThreadPool<uint64_t> thread_pool_;
+ utils::ThreadPool<utils::TaskRescheduleInfo> &thread_pool_;
Review comment:
Normally a raw pointer is only dangerous if the lifetime or ownership is
unclear or the (clear) contract is violated somewhere.
**Ownership:**
Since we use smart pointers all over the codebase to annotate ownership, all
owning pointers have a non-raw pointer type. This makes the currently
recommended practice of using raw pointers as non-owning pointers safe. We can
make the contract even more clear by introducing an annotation for observer
pointers, similarly to `gsl::owner`. See also
https://en.cppreference.com/w/cpp/experimental/observer_ptr
**Lifetime:**
`ThreadPool` is owned by `FlowController` which lives longer than
`SchedulingAgent`s. Check.
**Contract/ `delete` non-owning pointer:**
At the moment we don't have more than code reviews to check for these. In
2020, a `delete` in new code raises many eyebrows and attracts extra attention,
so I think we're safe here.
**Contract/Null:**
This is the most dangerous part is using a raw ptr for that member. Without
gsl, we don't have a safe way to annotate the pointer to be non-null. The most
we can do is add a comment next to the declaration. We're safe against null
references, since the vast majority of C++ programmers would reject `*nullptr`
instantly during code review, but `nullptr` is widely used and has valid use
cases, so we have no protection against null pointers in the proposed case.
Related talk:
https://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare/
Because of the `nullptr` issue, I'm fine with keeping `thread_pool_` as a
reference. The referenced point from the C++ Core Guidelines is just a note,
not a properly explained guideline, so I'm even fine with ignoring it.
edit: formatting, because of the length
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
