Conversions between openDAQ and C for openDAQ core types #7
Conversation
tomaz-cvetko
left a comment
tomaz-cvetko
left a comment
There was a problem hiding this comment.
Looks good in general, it would be good to rename some conversion functions to always have consistent naming in alignment with the direction of conversion.
There is also some discussion to be had regarding sideways interface casting - maybe I need to be educated what is possible there and what is not.
| daqList_createList(&names); | ||
|
|
||
| daqList_pushBack(names, (daqBaseObject*) stringOpenDAQConversion("Error")); | ||
| daqList_pushBack(names, (daqBaseObject*) stringOpenDAQConversion("Ok")); | ||
| daqList_pushBack(names, (daqBaseObject*) stringOpenDAQConversion("Warning")); | ||
|
|
||
| daqEnumerationType* enumType = NULL; | ||
| daqEnumerationType_createEnumerationType(&enumType, stringOpenDAQConversion("DAQ_ComponentStatusTypeEnum"), names, 0); | ||
|
|
||
| daqTypeManager_addType(typeManager, (daqType*) enumType); | ||
| daqReleaseRef(enumType); |
There was a problem hiding this comment.
This enum is already present in openDAQ. This is now just duplicating it. We need the struct type, since no other representative struct types exist.
| * from C to openDAQ a pointer to the TypeManager is needed because | ||
| * of the way openDAQ structs are implemented. | ||
| */ | ||
| struct Coordinates openDAQCoordinatesStructConversion(daqStruct* daq) |
There was a problem hiding this comment.
The names of the objects should be something more descriptive.
| * from C to openDAQ a pointer to the TypeManager is needed because | ||
| * of the way openDAQ enumeration is implemented. | ||
| */ | ||
| enum ComponentStatusTypeEnum openDAQEnumConversion(daqEnumeration* daq) |
There was a problem hiding this comment.
As with the struct conversion, this is an example not an utility method. This header should have a corresponding example that calls these helper methods. The enum and struct ones should not have any helpers, there should be examples only. The examples should also include creating a struct using the structBuilderFromStruct factory (name might be slightly different).
| daqString_getCharPtr(str, &cString); | ||
| return cString; | ||
| } | ||
| char* daq_fromDaqString(daqString* daq); |
There was a problem hiding this comment.
This function needs a separate docstring that explains whether or not the user should call free on the returned pointer. The classic C pattern would suggest that the caller must do the following
char* p = daq_fromDaqString(string);
// ...
if (p)
free(p);
but I think that's not the case (?). Does this function return a non-owning pointer to an internal buffer?
| daqFloatObject_getValue(value, &cDouble); | ||
| return cDouble; | ||
| } | ||
| double daq_fromDaqObject(daqFloatObject* daq); |
There was a problem hiding this comment.
There needs to be "float" in this function name. daq_fromDaqFloatObject?
| daqList_pushBack(names, (daqBaseObject*) daq_toDaqString("x")); | ||
| daqList_pushBack(names, (daqBaseObject*) daq_toDaqString("y")); | ||
| daqList_pushBack(names, (daqBaseObject*) daq_toDaqString("z")); | ||
|
|
There was a problem hiding this comment.
While it looks good, I think this is not okay, because you don't call daqReleaseRef on the pointer resulting from daq_toDaqString. While the conversions make creation automated, these are still not smart pointers and won't release resources when going out of scope.
- literal "x" is "created"
- in
daq_toDaqStringa new object is assigned to the pointer withdaqString_createStringand the pointer is returned. - The newly created object is used inside the
daqList_pushBackvia the pointer passed by value. - [MISSING] The newly created object is never released (at the moment that's not even possible, because the only reference to it is a temporary passed directly to the
pushBackfunction.
Important: every use of conversion function daq_toDaq____ must have a daqReleaseRef pair.
Conversions for: