zserio icon indicating copy to clipboard operation
zserio copied to clipboard

Mandatory fields

Open reinco opened this issue 3 years ago • 7 comments

In some cases BitStreamWriter throws an error if a mandatory field is not filled. In the NDS joint dev standup we were however unsure what happens with primitive types (like boolean flags, integers). If a mandatory field of such a primitive type is not set, is an exception thrown as well?

It is important that fields are not set to a default value of the underlying c++ type, because it is not sure whether that value makes sense in our product.

Consider for example the following scenario: we do an upgrade to a newer version of a zserio script file and one of the structs has an additional field. If in our NDS compiler we get no error while writing the data to a blob, we might overlook the change, if we populate a structure by calling its individual field setter functions.

reinco avatar Oct 21 '21 10:10 reinco

All built-in zserio types (like uint8) which are mapped to C++ primitive types (like uint8_t) are initialized by calling of default constructor. Which actually means that such types are initialized by zero. This is even valid for enumerations and bitmasks.

The BitStreamWriter after default construction can throw

  • if optional field is used but not set
  • if choice has not set any member
  • if union has not set any member
  • if zero is not valid value for primitive types (e.g. constraint) or for enumerations and bitmasks

On another hand, I understand your scenario and see your point. Unfortunately, there is currently no way how to distinguish between default value (zero) and already set valid value (can be zero as well).

If this would be needed, we will have to introduce extra boolean isSet for all members in compound types. But this will be not acceptable for "reader-only" applications because this will increase memory consumption by unnecessary booleans. Because of that, we will have to probably redesign significantly generated code to separate writer and reader part in different way. As an example of such separation, you might see issue https://github.com/ndsev/zserio/issues/203.

mikir avatar Oct 22 '21 07:10 mikir

Maybe it makes sense to only add it to writer code then and include it in the command line option withWriterCode whether it shall be generated or not.

fklebert avatar Oct 28 '21 08:10 fklebert

Yes, this could be an option as well if we don't have time to implement something better as described in #203.

We should probably change Java and Python generators in the same way as well not to introduce some new inconsistencies.

mikir avatar Nov 01 '21 09:11 mikir

In the meantime, we can suggest as workaround to use always field constructor. This will ensure that all fields are initialized at once and any new additional field will cause compilation error.

mikir avatar Mar 16 '22 15:03 mikir

How can this one be covered considering the Storage-View approach?

MisterGC avatar Apr 22 '24 09:04 MisterGC

Looking at the zserio documentation, there is a way to specify a default value to use. It does not say anything about what value to use if there is no explicit default.

It therefore may make sense to use a std::optional for any field of a compound type that has no explicit default value.

We could additionally have a parameter to the zserio compiler to force setting all compound members (so generate a std::optional for any field of a compound type). That would include arrays (an empty array should not be considered identical to an unitialized array). Alternatively, we could also enforce that at least in the nds zserio modules we allow no explicit default values to prevent functional safety risks. That would mean that all compound struct members are a std::optional.

reinco avatar Apr 22 '24 10:04 reinco

Looking at the zserio documentation, there is a way to specify a default value to use. It does not say anything about what value to use if there is no explicit default.

It therefore may make sense to use a std::optional for any field of a compound type that has no explicit default value.

We could additionally have a parameter to the zserio compiler to force setting all compound members (so generate a std::optional for any field of a compound type). That would include arrays (an empty array should not be considered identical to an unitialized array). Alternatively, we could also enforce that at least in the nds zserio modules we allow no explicit default values to prevent functional safety risks. That would mean that all compound struct members are a std::optional.

I would prefer NOT to add yet another parameter to the Zserio compiler. This will only make things more and more complicated. With too many Zserio compiler parameters, testing all possible combinations of generated code becomes almost impossible.

My opinion is that Storage should be a storage without any modification via parameters. If more functionality is needed, like mandatory fields, this can be implemented on the top, e.g. in the special class Builder which will allow to create Storage with additional checkings (as the next step after implementation because of effort).

mikir avatar Apr 22 '24 11:04 mikir