Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/432#discussion_r231884707
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -323,55 +360,45 @@ int transmit_flowfile(flow_file_record *ff,
nifi_instance *instance) {
flow * create_new_flow(nifi_instance * instance) {
auto minifi_instance_ref =
static_cast<minifi::Instance*>(instance->instance_ptr);
- flow *new_flow = (flow*) malloc(sizeof(flow));
-
- auto execution_plan = new
ExecutionPlan(minifi_instance_ref->getContentRepository(),
minifi_instance_ref->getNoOpRepository(),
minifi_instance_ref->getNoOpRepository());
-
- new_flow->plan = execution_plan;
-
- return new_flow;
+ return new flow(minifi_instance_ref->getContentRepository(),
minifi_instance_ref->getNoOpRepository(),
minifi_instance_ref->getNoOpRepository());
--- End diff --
Multiple thins to add there:
- Please note that this is not an introduced "new" call, just the one 5
lines above.
- We can change this to use malloc and placement new, in which case it
ensures that free only causes some leak, but no crash.
- I don't think this approach is sustainable in long term. To properly free
structures the users would be required to know the internal layout of the
structure, which is not going to happen and it would lead to very complex free
strategies in case of shared ownerships. Moreover I don't think that the
approach "feel free to free, it's just a leak" is the way forward. One of the
reasons I only left declarations in cstructs.h is that freeing something you
don't even know should make you raise concerns. What I would really push is the
user freeing up all the stuff he allocated and we provide free functions for
every structure that the API may return. Don't free what you don't allocate is
quite clear and well defines responsibilities. To go even further here, letting
the users free things might be a painful barrier in case we plan to change the
internal representation, which they shouldn't be aware of anyway.
In nutshell, combining free with placement new is good enough to avoid
immediate crashes, but we should try to highlight that the structures allocated
by the API should also be freed by that.
---
