You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by phrocker <gi...@git.apache.org> on 2018/11/12 18:13:19 UTC
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
GitHub user phrocker opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436
MINIFICPP-667: Add structural definitions to work out plan for C migr…
…ation
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/phrocker/nifi-minifi-cpp MINIFICPP-667
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/436.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #436
----
commit afe8fedac949270f408227ced9f8bb3bbd6cc09b
Author: Marc Parisi <ph...@...>
Date: 2018-11-12T18:12:40Z
MINIFICPP-667: Add structural definitions to work out plan for C migration
----
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233919866
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
Okay, fine for me. 👍 Happy to approve as it gets moved.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233910986
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -72,19 +41,17 @@ class DirectoryConfiguration {
* nifi_instance *create_instance(nifi_port const *port) {
*/
nifi_instance *create_instance(const char *url, nifi_port *port) {
- // make sure that we have a thread safe way of initializing the content directory
- DirectoryConfiguration::initialize();
-
- // need reinterpret cast until we move to C for this module.
- nifi_instance *instance = reinterpret_cast<nifi_instance*>(malloc(sizeof(nifi_instance)));
+ // need reinterpret cast until we move to C for this module.
+ nifi_instance *instance = (nifi_instance*)(malloc(sizeof(nifi_instance)));
/**
* This API will gradually move away from C++, hence malloc is used for nifi_instance
* Since minifi::Instance is currently being used, then we need to use new in that case.
*/
- instance->instance_ptr = new minifi::Instance(url, port->port_id);
+ instance->instance_ptr = create_cxx_instance(url,port);
// may have to translate port ID here in the future
// need reinterpret cast until we move to C for this module.
- instance->port.port_id = reinterpret_cast<char*>(malloc(strlen(port->port_id) + 1));
+ instance->port.port_id = (char*)(malloc(strlen(port->port_id) + 1));
--- End diff --
I expect this to fail linter, do we already have nanofi out of range of that?
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233910142
--- Diff: nanofi/include/core/cstructs.h ---
@@ -76,6 +80,12 @@ typedef struct {
* ##################################################################
*/
+typedef struct{
+ char *key;
--- End diff --
Can we have this as const char *?
A bit better user experience if you can write something like
```
key_value.key = "some string"
```
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r234011794
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
As per the title this is to work out a plan so that it's out in the open on Apache. I won't be merging this.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233911441
--- Diff: nanofi/src/api/nanofi.cpp ---
@@ -72,19 +41,17 @@ class DirectoryConfiguration {
* nifi_instance *create_instance(nifi_port const *port) {
*/
nifi_instance *create_instance(const char *url, nifi_port *port) {
- // make sure that we have a thread safe way of initializing the content directory
- DirectoryConfiguration::initialize();
-
- // need reinterpret cast until we move to C for this module.
- nifi_instance *instance = reinterpret_cast<nifi_instance*>(malloc(sizeof(nifi_instance)));
+ // need reinterpret cast until we move to C for this module.
+ nifi_instance *instance = (nifi_instance*)(malloc(sizeof(nifi_instance)));
/**
* This API will gradually move away from C++, hence malloc is used for nifi_instance
* Since minifi::Instance is currently being used, then we need to use new in that case.
*/
- instance->instance_ptr = new minifi::Instance(url, port->port_id);
+ instance->instance_ptr = create_cxx_instance(url,port);
// may have to translate port ID here in the future
// need reinterpret cast until we move to C for this module.
- instance->port.port_id = reinterpret_cast<char*>(malloc(strlen(port->port_id) + 1));
+ instance->port.port_id = (char*)(malloc(strlen(port->port_id) + 1));
--- End diff --
Yes. We won't be able to use the C++ linter on C files. I haven't pushed the recent commit, but I'm moving toward renaming this to nanofi.c
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233909297
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
I would prefer to place this file somewhere else, as it's not really part of core, but a 3rd party library we use. Could you add to a folder that implies this?
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233910857
--- Diff: nanofi/include/core/cstructs.h ---
@@ -76,6 +80,12 @@ typedef struct {
* ##################################################################
*/
+typedef struct{
+ char *key;
--- End diff --
The example code I took this from had this as char*, not sure we can do this. I'd have to look at the library.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233913016
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
Ah thanks for calling this out. I have a more recent commit with a lot of changes, but didn't move this yet and probably would have forgotten.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233913139
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
We have a third party dir in the main directory, which is where this should go.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233913935
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
Not yet in nanofi :)
Guess this library is not going to be used in the C++ part.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233919353
--- Diff: nanofi/include/core/utlist.h ---
@@ -0,0 +1,1073 @@
+/*
--- End diff --
sorry CMAKE_SOURCE_DIR. It can go there and should. I think I updated the NOTICE and we direct people to the root source thirdparty, so we can probably just use that. No reason to create levels of thirdparty source dirs yet.
---
[GitHub] nifi-minifi-cpp pull request #436: MINIFICPP-667: Add structural definitions...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/436#discussion_r233911949
--- Diff: nanofi/include/core/cstructs.h ---
@@ -76,6 +80,12 @@ typedef struct {
* ##################################################################
*/
+typedef struct{
+ char *key;
--- End diff --
I'll add that I'm not against this, but the code pointer's management was a little different. Again, I'll take a look at if this makes sense. core will be things that don't reflect a user experience though. That is code that's not part of our public API.
---