PengZheng commented on code in PR #699:
URL: https://github.com/apache/celix/pull/699#discussion_r1472768662
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type
*type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
Review Comment:
After consulting gcc's manual ([1]) and C/C++ standards ([2]), my conclusion
is: we'd better stick with sint32 until C23 becomes widely used. It's
unfortunate that enumeration of fixed underlying type won't be available to C
until C23, which is at least five years away.
That is not to say we are not able to help our users to avoid pitfalls. For
example, we can provide a auxiliary macso that can be used to decorate enum
declarations. Once decorated, an implicit enumerator will be inserted in the
end of enumerator-list with a value of `INT32_MAX` together with a build-time
assertion claiming `sizeof(enum)==4`. That should help catch most errors of
this kind, like the ones caused by GCC option `-fshort-enums`.
I suggest we reconsider this when implementing descriptor generator. At that
time, we are at a better position, since we can see things from a point of view
of compiler. IMHO, such rules as the ones current implemented by
dyn_function/dyn_interface/serializer should be enforced as much as possible by
the descriptor generator.
[1]
https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
[2] https://en.cppreference.com/w/c/language/enum
##########
libs/dfi/src/dyn_type.c:
##########
@@ -185,265 +187,200 @@ static int dynType_parseAny(FILE *stream, dyn_type
*type) {
return status;
}
-static int dynType_parseMetaInfo(FILE *stream, dyn_type *type) {
+static int dynType_parseMetaInfo(FILE* stream, dyn_type* type) {
int status = OK;
- char *name = NULL;
- char *value = NULL;
-
- struct meta_entry *entry = calloc(1, sizeof(*entry));
- if (entry == NULL) {
- status = ERROR;
- }
+ celix_autofree char* name = NULL;
+ celix_autofree char* value = NULL;
- if (status == OK) {
- status = dynCommon_parseName(stream, &name);
+ if (dynCommon_parseName(stream, &name) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, '=');
+ if (dynCommon_eatChar(stream, '=') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_parseName(stream, &value);
+ if (dynCommon_parseName(stream, &value) != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- status = dynCommon_eatChar(stream, ';');
+ if (dynCommon_eatChar(stream, ';') != OK) {
+ status = PARSE_ERROR;
+ goto bail_out;
}
-
- if (status == OK) {
- entry->name = name;
- entry->value = value;
- TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
- } else {
- LOG_ERROR("Failed to parse meta properties");
- free(name);
- free(value);
- free(entry);
+ struct meta_entry *entry = calloc(1, sizeof(*entry));
+ if (entry == NULL) {
+ status = MEM_ERROR;
+ goto bail_out;
}
+ entry->name = celix_steal_ptr(name);
+ entry->value = celix_steal_ptr(value);
+ TAILQ_INSERT_TAIL(&type->metaProperties, entry, entries);
+ return OK;
+bail_out:
+ celix_err_push("Failed to parse meta properties");
return status;
}
-static int dynType_parseText(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseText(FILE* stream, dyn_type* type) {
type->type = DYN_TYPE_TEXT;
type->descriptor = 't';
+ type->trivial = false;
type->ffiType = &ffi_type_pointer;
- return status;
+ return OK;
}
-static int dynType_parseEnum(FILE *stream, dyn_type *type) {
- int status = OK;
+static int dynType_parseEnum(FILE* stream, dyn_type* type) {
type->ffiType = &ffi_type_sint32;
Review Comment:
After consulting GCC's manual ([1]) and C/C++ standards ([2]), my conclusion
is: we'd better stick with sint32 until C23 becomes widely used. It's
unfortunate that enumeration of fixed underlying type won't be available to C
until C23, which is at least five years away.
That is not to say we are not able to help our users to avoid pitfalls. For
example, we can provide a auxiliary macso that can be used to decorate enum
declarations. Once decorated, an implicit enumerator will be inserted in the
end of enumerator-list with a value of `INT32_MAX` together with a build-time
assertion claiming `sizeof(enum)==4`. That should help catch most errors of
this kind, like the ones caused by GCC option `-fshort-enums`.
I suggest we reconsider this when implementing descriptor generator. At that
time, we are at a better position, since we can see things from a point of view
of compiler. IMHO, such rules as the ones current implemented by
dyn_function/dyn_interface/serializer should be enforced as much as possible by
the descriptor generator.
[1]
https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
[2] https://en.cppreference.com/w/c/language/enum
--
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
