Follow-up from "Stack overflow fix"
The following discussions from !545 (merged) should be addressed:
-
@brameret started a discussion: (+2 comments) (there are too many changes in sopc_encoder for gitlab to display them, and to be able to comment them)
In
sopc_encoder.c:195
, the early return was changed to astatus =
statement. It is clearer to use early returns in those functions. I agree it is better to homogenize them, but it would be even better to homogenize them towards the clearer option. In this case,SOPC_Boolean_Write
could become:SOPC_ReturnStatus SOPC_Boolean_Write(const SOPC_Boolean* value, SOPC_Buffer* buf, uint32_t nestedStructLevel) { if (NULL == value || NULL == buf) { return SOPC_STATUS_INVALID_PARAMETERS; } else if (nestedStructLevel > SOPC_MAX_STRUCT_NESTED_LEVEL) { return SOPC_STATUS_INVALID_STATE; } SOPC_Byte encodedValue; nestedStructLevel++; if (false == *value) { encodedValue = *value; } else { // Encoder should use 1 as True value encodedValue = 1; } return SOPC_Byte_Write(&encodedValue, buf, nestedStructLevel); }
-
@brameret started a discussion: (+2 comments) (there are too many changes in sopc_encoder for gitlab to display them, and to be able to comment them)
In
sopc_encoder.c
, there are functions that incrementnestedStructLevel
, but other that don't.The ones that increment it:
- SOPC_Byte_Write
- SOPC_Boolean_Write
- SOPC_Boolean_Read
- SOPC_SByte_Write
- SOPC_Int16_Write
- SOPC_UInt16_Write
- SOPC_Int32_Write
- SOPC_UInt32_Write
- SOPC_Int64_Write
- SOPC_UInt64_Write
- SOPC_Float_Write
- SOPC_Double_Write
- SOPC_ByteString_Write
- SOPC_ByteString_Read
- SOPC_String_Write
- SOPC_String_ReadWithLimitedLength
- SOPC_DateTime_Write
- SOPC_DateTime_Read
- SOPC_Guid_Write
- SOPC_Guid_Read
- SOPC_NodeId_Write
- SOPC_NodeId_Read
- SOPC_ExpandedNodeId_Write
- SOPC_ExpandedNodeId_Read
- SOPC_QualifiedName_Write
- SOPC_QualifiedName_Read
- SOPC_LocalizedText_Write
- SOPC_LocalizedText_Read
- SOPC_ExtensionObject_Write
- SOPC_ExtensionObject_Read
- ...
The ones that don't:
- SOPC_Byte_Read
- SOPC_SByte_Read
- SOPC_Int16_Read
- SOPC_UInt16_Read
- SOPC_Int32_Read
- SOPC_UInt32_Read
- SOPC_UInt64_Read
- SOPC_Int64_Read
- SOPC_Float_Read
- SOPC_Double_Read
- SOPC_DiagnosticInfo_Write
- SOPC_DiagnosticInfo_Read
- ...
I think these should be homogenize. I don't understand why some reads don't increment the nestedStructLevel. I think this could lead to structures that would be decodable but not encodable (
sopc_encodEabletype.c
?).There are also some non homogenized behaviors in NodeId:
- SOPC_NodeId_Write and SOPC_ExpandedNodeId_Write increment the nestedStructLevel only once,
- SOPC_NodeId_Read increments it twice and SOPC_ExpandedNodeId_Read increments it thrice.
And in several other points, such as in SOPC_DataValue_Write, which tests nestedStructLevel before calling the _Internal static function, whereas SOPC_Variant_Read does not. DataValue_Write could be simplified to just return _Internal(...), as it is done in SOPC_Variant_Read.
-
@brameret started a discussion: You re-introduced some
if(var [op] const)
:)I know
!=
are less likely to be missed than==
, but still, that's a comparison with the equal sign. -
@brameret started a discussion: You can declare the variable here to avoid its initialization to an unused value.
(this applies to all functions in this file)