OleksandrKvl / sbepp

C++ implementation of the FIX Simple Binary Encoding
https://oleksandrkvl.github.io/sbepp/
MIT License
39 stars 4 forks source link

reset member function for the optional types #27

Closed ujos closed 4 months ago

ujos commented 4 months ago

Could you please add the reset() member function to the optional classes (e.g. uint8_opt_t) to reset the value into NULL.

ujos commented 4 months ago

Looks like it is not really possible, as msg.field1() returns a copy of the value. E.g. there is no sense to modify the copy.

From other side what is the other way?

msg.field1(XXX::null_value()); // What should I put instead of XXX?

In case if field is of sbepp::uint8_opt_t, instead of XXX I can put uint8_opt_t. But, what will happen if later I change the type of the field1 from uint8 to uint16? Compiler won't complain about type incompatibility.

The only way I see is :

m.field1(m.field1().null_value());
ujos commented 4 months ago

Another solution is to add m.reset_field1() member function. I'm not quite sure about what format to choose:

ujos commented 4 months ago

BTW, enums can have null value too.

<type name="uInt8NULL" description="uInt8NULL" presence="optional" nullValue="255" primitiveType="uint8"/>
<enum name="AggressorSide" encodingType="uInt8NULL">
  <validValue name="NoAggressor" description="No Aggressor">0</validValue>
  <validValue name="Buy" description="Buy">1</validValue>
  <validValue name="Sell" description="Sell">2</validValue>
</enum>

Would be nice to have reset() and null_value() member function somehow for the enums too.

OleksandrKvl commented 4 months ago

No, SBE doesn't allow optional enums, that's for sure.

The simplest way to set field to a null value is msg.optionalField({}); or msg.optionalField(sbepp::nullopt);.

ujos commented 4 months ago

The simplest way to set field to a null value is msg.optionalField({}); or msg.optionalField(sbepp::nullopt);.

if I do so, I get compile time error:

build] /home/dev/conan/data/sbepp/1.2.0/_/_/package/e56740c5c8607604ce400c99ed33beb0e12c2a18/include/
  sbepp/sbepp.hpp:3560:39: error: ‘null_value’ is not a member of ‘sbepp::uint8_t’

Can it be caused by the following line, so instead of NAME##_opt_t the second template parameter is NAME##_t ?

class NAME##_opt_t : public detail::optional_base<TYPE, NAME##_t>
OleksandrKvl commented 4 months ago

That's definitely a bug. Try to create your own optional type like <type name="uint32_opt" primitiveType="uint32" presence="optional" minValue="0" maxValue="10" nullValue="11"/> and set it to null like I explained before.

ujos commented 4 months ago

Try to create your own optional type like

I'm looking at specification and actually looks like "presence" attribute can be applied only to "field" and to the "type" inside the "composite". Weird...

UPD: https://github.com/FIXTradingCommunity/fix-simple-binary-encoding/pull/139

ujos commented 4 months ago

No, SBE doesn't allow optional enums, that's for sure.

I found your ticket https://github.com/FIXTradingCommunity/fix-simple-binary-encoding/issues/153 .

Something is not in line. Because <field> tag can have a presence attribute, enum field actually can be optional...

ujos commented 4 months ago

The simplest way to set field to a null value is msg.optionalField({}); or msg.optionalField(sbepp::nullopt);.

Would be nice to have an option to pass std::nullopt too.

ujos commented 4 months ago

Regarding the nullable Enums. Following is a quote from the SBE specification

Boolean encoding

A Boolean field is a special enumeration with predefined valid values: true and false. Like a standard enumeration, an optional Boolean field may have nullValue that indicates that the field is null (or not applicable).

Standard encoding specifications for required and optional Boolean fields

<enum name="booleanEnum" encodingType="uint8">
    <validValue name="false">0</validValue>
    <validValue name="true">1</validValue>
</enum>

Example optional Boolean field

<field type="requiredBoolean" name="SolicitedFlag" id="377" presence="required" semanticType="Boolean" />

<field type="optionalBoolean" name="SolicitedFlag" id="377" presence="optional" nullValue="255" semanticType="Boolean" />
OleksandrKvl commented 4 months ago

I don't know what documentation you're referring to but here's the screenshot from PDF I use:

Screenshot 2024-03-21 at 15 50 27

There's no design space for optional enums and sets (and even for composites), because they can't have a reserved value for null representation. If you think otherwise, go to the official repo and get confirmation from them, this project is not the right place to discuss such details. SBE specification is pretty bad, it has a lot of such gaps.

ujos commented 4 months ago

Yours is exactly as mine one. I read the Markdown file from the git repo.

See in bold: Like a standard enumeration, an optional Boolean field may have nullValue that indicates that the field is null (or not applicable).

ujos commented 4 months ago

https://github.com/FIXTradingCommunity/fix-simple-binary-encoding/blob/master/v2-0-RC3/doc/02FieldEncoding.md#boolean-encoding

OleksandrKvl commented 4 months ago

Yours is exactly as mine one. I read the Markdown file from the git repo.

See in bold: Like a standard enumeration, an optional Boolean field may have nullValue that indicates that the field is null (or not applicable).

That nullValue is a plain enumerator, not the presence property of the enum.

OleksandrKvl commented 4 months ago

https://github.com/FIXTradingCommunity/fix-simple-binary-encoding/blob/master/v2-0-RC3/doc/02FieldEncoding.md#boolean-encoding

2.0 is just a draft, which is still badly specified because it contradicts to itself. What prevents me from:

<enum name="booleanEnum" encodingType="uint8">
    <validValue name="false">0</validValue>
    <validValue name="true">1</validValue>
    <validValue name="magic">255</validValue>
</enum>

?

ujos commented 4 months ago

Dunno... on your question they had replied partially. @donmendelson did mention that enum cannot be nullable. In fact it is possible and they propose (as stated in the Specification) to use "nullValue" validValue.

Ok, let's keep this conversation about nullable enums outside of this ticket. If you want, you may add support of nullValue attribute of the <field> tag or <validValue name="nullValue"... tag to the generator so one can do:

msg.optionalFlag(std:nullopt);

That nullValue is a plain enumerator, not the presence property of the enum.

According to SBE spec v2.0-RC3 <enum> tag cannot have a presence attribute. <field> can. Maybe SBE v1.0 allows that attroibute. I did not check.

2.0 is just a draft

It is release candidate

OleksandrKvl commented 4 months ago

Fixed built-in optional types. Closing this as there's not enough motivation at the moment for reset().

you may add support of nullValue attribute of the tag or <validValue name="nullValue"

This is a hack that relies on a specific name.

Would be nice to have an option to pass std::nullopt too.

"Would be nice" is not enough to introduce a dependency on <optional> that's available only in C++17.

It is release candidate

... that has not been updated for 4 years (and was not intended for ISO anyway).

ujos commented 4 months ago

Fixed built-in optional types. Closing this as there's not enough motivation at the moment for reset().

setter for the nullopt is totally fine, thank you

This is a hack that relies on a specific name.

Right. I'm not sure if anyone would use that name for anything else but for the null value. I would rely on that name even if it is not in standard. At the end it is matter of convenience. That won't harm but will make the API more user friendly.

"Would be nice" is not enough to introduce a dependency on that's available only in C++17.

The dependency can be introduced conditionally using #if __cplusplus >= abcd. I can do that. I would love to have more integration with the core C++.

Thank you @OleksandrKvl for assistance and for the quick fix.