MINIFICPP-667: Add structural definitions to work out plan for C migr…#436
MINIFICPP-667: Add structural definitions to work out plan for C migr…#436phrocker wants to merge 2 commits intoapache:masterfrom
Conversation
| @@ -0,0 +1,1073 @@ | |||
| /* | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We have a third party dir in the main directory, which is where this should go.
There was a problem hiding this comment.
Not yet in nanofi :)
Guess this library is not going to be used in the C++ part.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, fine for me. 👍 Happy to approve as it gets moved.
There was a problem hiding this comment.
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 quite yet.
| */ | ||
|
|
||
| typedef struct{ | ||
| char *key; |
There was a problem hiding this comment.
Can we have this as const char *?
A bit better user experience if you can write something like
key_value.key = "some string"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
In general with C APIs I either allow value and pointer to be changed or prevent both with const char * const key -- so I'll take a look in the recent commit what I've done. I don't recall as I'm working on a more pressing bug now.
| // 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)); |
There was a problem hiding this comment.
I expect this to fail linter, do we already have nanofi out of range of that?
There was a problem hiding this comment.
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
| }; | ||
|
|
||
| int API_INITIALIZER::initialized = initialize_api(); | ||
|
|
There was a problem hiding this comment.
@arpadboda I'm going to merge this PR since some of this has made it's way elsewhere and this was just a proof of concept; however, it may make sense to begin isolating the cpp layer a little more like this for the eventuality that we remote it entirely.
There was a problem hiding this comment.
You mean NOT going to merge?
Totally agree, my current site2site work introduces UT collections anyway to help working in C.
|
Was simply proof of concept to work out plan and visually see ideas in a diff. never intended to merge, so closing as it's OBE. |
| */ | ||
| typedef struct { | ||
| // structural definitions for moving way from C++ | ||
| unsigned modified:1; |
There was a problem hiding this comment.
@arpadboda changes in cstructs were the only thing of merit here, where we have attributes...are you incorporating something like this into your site to site work?
There was a problem hiding this comment.
No, didn't touch C structures yet, I think I won't need to do either as there is C API to get attributes.
2 participants
…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:
For documentation related changes:
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.