pnoltes commented on code in PR #782:
URL: https://github.com/apache/celix/pull/782#discussion_r1921998913
##########
libs/utils/include/celix_properties.h:
##########
@@ -1199,7 +1199,8 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_loadFromStream(FILE* stream,
*
* For a overview of the possible decode flags, see the
CELIX_PROPERTIES_DECODE_* flags documentation.
*
- * If an error occurs, the error status is returned and a message is logged to
celix_err.
+ * If an error occurs, the error status is returned and a message is logg
Review Comment:
nitpick: word logged is split over 2 lines
##########
libs/dfi/README.md:
##########
@@ -71,9 +71,9 @@ The data types supported by the interface description include:
*Type schema*:
- |**Identifier**|B |D |F |I |J |S |V |Z
|b | i | j | s |P | t |N |
-
|---------|---|------|-----|-------|-------|-------|----|--------------|-----|--------|--------|--------|------|------------------|---|
-
|**Types**|char|double|float|int32_t|int64_t|int16_t|void|boolean(uint8)|uchar|uint32_t|uint64_t|uint16_t|void
*| char *(C string) |int|
+ |**Identifier**|B |D |F |I |J |S |V |Z
|b | i | j | s |P | t |N | p
| a |
Review Comment:
Is a descriptor `a` enough for array list?
With the current implementation of array list, the array element type is
fixed at creation. Using the array list wrong (e.g. getLong, when it is a
string) will lead to an assert failure.
With dfi you can serializer / deserialize array list of any element type,
but the underlying code will assume a element type (or needs to use defense
programming by checking the element type before continuing).
Maybe it is better to use different dfi descriptor for the different array
lists (string, long, double, boolean and version).
@xuzhenbao and @PengZheng WDYT?
##########
libs/dfi/README.md:
##########
@@ -236,12 +236,12 @@ The data types supported by the interface description
include:
~~~
In order to represent the properties of function parameters (eg: in,
out...), function parameters support the following metadata annotations:
- |Meta-info| Description|
- |---------|------------|
- |am=handle| void pointer for the handle.|
- |am=pre | output pointer with memory pre-allocated, it should be pointer
to [trivially copyable type](#notion-definitions).|
- |am=out | output pointer, the caller should use `free` to release the
memory, and it should be pointer to text(t) or double pointer to [serializable
types](#notion-definitions).|
- |const=true| text argument(t) can use it, Normally a text argument will be
handled as char*, meaning that the callee is expected to take of ownership.If a
const=true annotation is used the text argument will be handled as a const
char*, meaning that the caller keeps ownership of the string.|
+ |Meta-info| Description
|
+
|---------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+ |am=handle| void pointer for the handle.
|
+ |am=pre | output pointer with memory pre-allocated, it should be pointer
to [trivially copyable type](#notion-definitions).
|
+ |am=out | output pointer, the caller should use `free` to release the
memory, and it should be pointer to text(t) or double pointer to [serializable
types](#notion-definitions).
|
+ |const=true| text argument(t) and `celix_properties_t*`(p) and
`celix_array_list_t*`(a) can use it, Normally a text argument will be handled
as char*, meaning that the callee is expected to take of ownership.If a
const=true annotation is used the text argument will be handled as a const
char*, meaning that the caller keeps ownership of the string. |
Review Comment:
Nitpick: The properties and array_list types have been added as potential
targets for `const=true`, but the accompanying text currently only references
`char*`. I suggest we rephrase it for clarity, something like:
```
Normally, a text, properties, or array list argument will be handled
respectively as `char*`, `celix_properties_t*`, or `celix_array_list_t*`,
implying that the callee is expected to take ownership. However, if the
`const=true` annotation is used, these arguments will be handled as `const
char*`, `const celix_properties_t*`, or `const celix_array_list_t*`, indicating
that the caller retains ownership of the string/object.
```
--
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: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
