szaszm commented on code in PR #1987:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1987#discussion_r2510647462
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
Review Comment:
Since C23, stdbool.h is deprecated, and `bool` is a built-in type.
```suggestion
#if __STDC_VERSION__ < 202311l
# include <stdbool.h>
#endif // < C23
```
##########
core-framework/c-api-framework/include/api/core/ProcessorImpl.h:
##########
Review Comment:
I would rename this component from "c-api-framework" to "cpp-extension-lib",
since it seems to be a C++ library for extension developers targeting the
MiNiFi C API.
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
-typedef struct MinifiConfig MinifiConfig;
-typedef struct MinifiExtension MinifiExtension;
+#define STRINGIFY_HELPER(X) #X
+#define STRINGIFY(X) STRINGIFY_HELPER(X)
+
+#define MINIFI_API_MAJOR_VERSION 0
+#define MINIFI_API_MINOR_VERSION 1
+#define MINIFI_API_PATCH_VERSION 0
+#define MINIFI_API_VERSION STRINGIFY(MINIFI_API_MAJOR_VERSION) "."
STRINGIFY(MINIFI_API_MINOR_VERSION) "." STRINGIFY(MINIFI_API_PATCH_VERSION)
+#define MINIFI_API_VERSION_TAG "MINIFI_API_VERSION=[" MINIFI_API_VERSION "]"
+#define MINIFI_NULL nullptr
+#define MINIFI_OWNED
+
+typedef bool MinifiBool;
+#define MINIFI_TRUE true
+#define MINIFI_FALSE false
Review Comment:
I don't think we need these, we can just use `true`/`false` directly: from
`stdbool.h` or from language keywords since C23.
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
-typedef struct MinifiConfig MinifiConfig;
-typedef struct MinifiExtension MinifiExtension;
+#define STRINGIFY_HELPER(X) #X
+#define STRINGIFY(X) STRINGIFY_HELPER(X)
+
+#define MINIFI_API_MAJOR_VERSION 0
+#define MINIFI_API_MINOR_VERSION 1
+#define MINIFI_API_PATCH_VERSION 0
+#define MINIFI_API_VERSION STRINGIFY(MINIFI_API_MAJOR_VERSION) "."
STRINGIFY(MINIFI_API_MINOR_VERSION) "." STRINGIFY(MINIFI_API_PATCH_VERSION)
+#define MINIFI_API_VERSION_TAG "MINIFI_API_VERSION=[" MINIFI_API_VERSION "]"
+#define MINIFI_NULL nullptr
+#define MINIFI_OWNED
+
+typedef bool MinifiBool;
+#define MINIFI_TRUE true
+#define MINIFI_FALSE false
+
+typedef enum MinifiInputRequirement {
+ MINIFI_INPUT_REQUIRED = 0,
+ MINIFI_INPUT_ALLOWED = 1,
+ MINIFI_INPUT_FORBIDDEN = 2
+} MinifiInputRequirement;
typedef struct MinifiStringView {
const char* data;
size_t length;
} MinifiStringView;
+typedef struct MinifiRelationship {
+ MinifiStringView name;
+ MinifiStringView description;
+} MinifiRelationship;
+
+typedef struct MinifiOutputAttribute {
+ MinifiStringView name;
+ size_t relationships_count;
+ const MinifiRelationship* relationships_ptr;
Review Comment:
This doesn't need a full copy of all the relationship metadata: a
relationship name as a unique identified would be enough here. I realize that
this contradicts my previous comment about separating static metadata and
runtime data, but a string view to a name seems to work both as a static
reference and a runtime one.
```suggestion
size_t relationships_count;
const MinifiRelationshipName* relationships_ptr;
```
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
-typedef struct MinifiConfig MinifiConfig;
-typedef struct MinifiExtension MinifiExtension;
+#define STRINGIFY_HELPER(X) #X
+#define STRINGIFY(X) STRINGIFY_HELPER(X)
+
+#define MINIFI_API_MAJOR_VERSION 0
+#define MINIFI_API_MINOR_VERSION 1
+#define MINIFI_API_PATCH_VERSION 0
+#define MINIFI_API_VERSION STRINGIFY(MINIFI_API_MAJOR_VERSION) "."
STRINGIFY(MINIFI_API_MINOR_VERSION) "." STRINGIFY(MINIFI_API_PATCH_VERSION)
+#define MINIFI_API_VERSION_TAG "MINIFI_API_VERSION=[" MINIFI_API_VERSION "]"
+#define MINIFI_NULL nullptr
+#define MINIFI_OWNED
+
+typedef bool MinifiBool;
+#define MINIFI_TRUE true
+#define MINIFI_FALSE false
+
+typedef enum MinifiInputRequirement {
+ MINIFI_INPUT_REQUIRED = 0,
+ MINIFI_INPUT_ALLOWED = 1,
+ MINIFI_INPUT_FORBIDDEN = 2
+} MinifiInputRequirement;
typedef struct MinifiStringView {
const char* data;
size_t length;
} MinifiStringView;
Review Comment:
We should have docs on all non-obvious parts. In this case, it's not obvious
whether data is nullable and whether it's null-terminated. I'd document, that
- data is nullable (I'm assuming)
- length represents the number of bytes pointed-to by data
- invariant: data is null if and only if length == 0.
- the bytes pointed-to by data may not be null-terminated, it is not a C
string
```suggestion
/// Represents a non-owning read-only view to a sized, not null-terminated
string.
/// invariant: `(data == NULL) == (length == 0)`, data is null if and only
if length is zero.
typedef struct MinifiStringView {
/// nullable, non-owning pointer to a not null-terminated string
const char* data;
/// the length of the buffer pointed-to by data
size_t length;
} MinifiStringView;
```
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
-typedef struct MinifiConfig MinifiConfig;
-typedef struct MinifiExtension MinifiExtension;
+#define STRINGIFY_HELPER(X) #X
+#define STRINGIFY(X) STRINGIFY_HELPER(X)
+
+#define MINIFI_API_MAJOR_VERSION 0
+#define MINIFI_API_MINOR_VERSION 1
+#define MINIFI_API_PATCH_VERSION 0
+#define MINIFI_API_VERSION STRINGIFY(MINIFI_API_MAJOR_VERSION) "."
STRINGIFY(MINIFI_API_MINOR_VERSION) "." STRINGIFY(MINIFI_API_PATCH_VERSION)
+#define MINIFI_API_VERSION_TAG "MINIFI_API_VERSION=[" MINIFI_API_VERSION "]"
+#define MINIFI_NULL nullptr
+#define MINIFI_OWNED
+
+typedef bool MinifiBool;
+#define MINIFI_TRUE true
+#define MINIFI_FALSE false
+
+typedef enum MinifiInputRequirement {
+ MINIFI_INPUT_REQUIRED = 0,
+ MINIFI_INPUT_ALLOWED = 1,
+ MINIFI_INPUT_FORBIDDEN = 2
+} MinifiInputRequirement;
typedef struct MinifiStringView {
const char* data;
size_t length;
} MinifiStringView;
+typedef struct MinifiRelationship {
+ MinifiStringView name;
+ MinifiStringView description;
+} MinifiRelationship;
+
+typedef struct MinifiOutputAttribute {
+ MinifiStringView name;
+ size_t relationships_count;
+ const MinifiRelationship* relationships_ptr;
+ MinifiStringView description;
+} MinifiOutputAttribute;
Review Comment:
I would add -Definition to the name to match the internal C++ API and to
make it clear that this is static metadata about the processor. Same with
MinifiRelationship above.
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
-typedef struct MinifiConfig MinifiConfig;
-typedef struct MinifiExtension MinifiExtension;
+#define STRINGIFY_HELPER(X) #X
+#define STRINGIFY(X) STRINGIFY_HELPER(X)
Review Comment:
Since macros are unscoped and these are not part of the API, I'd add hard to
type prefixes to discourage use outside of this header, or undef them after use.
```suggestion
// private macros, do not use outside of this header
#define MINIFI_PRIVATE_STRINGIFY_HELPER(X) #X
#define MINIFI_PRIVATE_STRINGIFY(X) MINIFI_PRIVATE_STRINGIFY_HELPER(X)
```
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -24,24 +24,193 @@ extern "C" {
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
-typedef struct MinifiConfig MinifiConfig;
-typedef struct MinifiExtension MinifiExtension;
+#define STRINGIFY_HELPER(X) #X
+#define STRINGIFY(X) STRINGIFY_HELPER(X)
+
+#define MINIFI_API_MAJOR_VERSION 0
+#define MINIFI_API_MINOR_VERSION 1
+#define MINIFI_API_PATCH_VERSION 0
+#define MINIFI_API_VERSION STRINGIFY(MINIFI_API_MAJOR_VERSION) "."
STRINGIFY(MINIFI_API_MINOR_VERSION) "." STRINGIFY(MINIFI_API_PATCH_VERSION)
+#define MINIFI_API_VERSION_TAG "MINIFI_API_VERSION=[" MINIFI_API_VERSION "]"
+#define MINIFI_NULL nullptr
+#define MINIFI_OWNED
+
+typedef bool MinifiBool;
+#define MINIFI_TRUE true
+#define MINIFI_FALSE false
+
+typedef enum MinifiInputRequirement {
+ MINIFI_INPUT_REQUIRED = 0,
+ MINIFI_INPUT_ALLOWED = 1,
+ MINIFI_INPUT_FORBIDDEN = 2
+} MinifiInputRequirement;
typedef struct MinifiStringView {
const char* data;
size_t length;
} MinifiStringView;
+typedef struct MinifiRelationship {
+ MinifiStringView name;
+ MinifiStringView description;
+} MinifiRelationship;
Review Comment:
We should separate static metadata from runtime data. The runtime data
version of Relationship doesn't need description.
```suggestion
/// Represents an output relationship of a processor, part of the processor
metadata
typedef struct MinifiRelationshipDefinition {
/// Name, processors use this to transfer flow files to the connections
connected to this relationship using ProcessSession transfer.
MinifiStringView name;
/// Human-readable description of what flow files get routed to this
relationship. Included in C2 manifest and generated processor docs.
MinifiStringView description;
} MinifiRelationshipDefinition;
typedef struct MinifiRelationshipName {
MinifiStringView name;
} MinifiRelationshipName;
```
##########
core-framework/c-api-framework/include/api/core/ProcessorImpl.h:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <atomic>
+#include <chrono>
+#include <condition_variable>
+#include <functional>
+#include <memory>
+#include <mutex>
+#include <string>
+#include <string_view>
+#include <unordered_set>
+#include <unordered_map>
+#include <utility>
+#include <vector>
+
+#include "minifi-cpp/core/Annotation.h"
+#include "minifi-cpp/core/DynamicProperty.h"
+#include "utils/gsl.h"
+#include "minifi-cpp/utils/Id.h"
+#include "minifi-cpp/core/OutputAttributeDefinition.h"
+#include "minifi-cpp/core/ProcessorMetadata.h"
Review Comment:
dependencies on MiNiFi C++ internals seems to defeat the purpose of this
lib, but maybe I'm misunderstanding things.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
