ProtoDef-io / node-protodef

Describe your protocol, and read it with ease.
MIT License
31 stars 18 forks source link

Option to fill in zero bytes for fixed size data types when undefined? #81

Open LiamKarlMitchell opened 7 years ago

LiamKarlMitchell commented 7 years ago

Oh, sorry about custom type it is just an example, I think this should be an option for any type that can have a count set to a fixed number.

If you have a definition with a fixed size.

{
  "name": "foo",
  "type": [
    "fcstring",
    {
      "count": 4,
      "trim": true
    }
  ]
}

And wanted to serialize some object where it is undefined {}

Then the buffer should zero fill.

<Buffer 00 00 00 00>

Example with buffer data type.

{
  "name": "foo",
  "type": [
    "buffer",
    {
      "count": 4
    }
  ]
}

<Buffer 00 00 00 00>

And applicable to arrays as well.

Would such an option be possible? Maybe when setting up the serializer.

hansihe commented 7 years ago

I disagree with this. If there is data missing that needs to be written, the serializer should throw a hard error.

Making guesses and zero-filling/doing anything else seems to me like it would make it really easy to introduce hard to debug logic errors. It is better to just throw an error, and the user can immediately see that they made a mistake and correct it.

Saiv46 commented 4 years ago

Maybe you should add "padding" property to datatype, but it would introduce various bugs (in case of "pstring" - it would deserialize to "\x00\x00\x00\x00", not empty string).

LiamKarlMitchell commented 4 years ago

Yeah I trim the null bytes so its not an issue. Sending buffers to a C++ made application with fixed sized structures so it wants this 0 padding.

Saiv46 commented 4 years ago

By default interpreter/compiler just ignores count and in future, it'll throw an error. https://github.com/ProtoDef-io/node-protodef/blob/806878f1202740d33c253de29f405c1eeca8bae8/src/utils.js#L31 https://github.com/ProtoDef-io/node-protodef/pull/107/files#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R86

@rom1504 @Karang Should we change this behavior?

roblabla commented 4 years ago

@LiamKarlMitchell For structure padding specifically, it might be worth thinking about some kind of data-type that's basically a "zeroed byte". The idea is that you'd never specify it yourself, it would always be automatically set to 0.

The concept could be more generally extended to setting default values for the existing primitives. E.G. those two would be equivalent:

struct mystruct {
    u16 a;
    // two bytes of padding due to C struct layout
    u32 b;
};
{
  "padding_byte": [
    "u8",
    {
      "value": 0
    }
  ],
  "mystruct": ["container", [
    { "name": "a", "type": "u16" },
    { "type": "padding_byte" },
    { "type": "padding_byte" },
    { "name": "b", "type": "u32" },
  ]]
}

@Saiv46 TBH, setting a count manually should never be allowed to begin with. When I designed count, I'm pretty sure I expected it to be derived automatically from the backing array all the time, and never be manually set. I'm a bit ashamed I left it as a TODO and never implemented the throw :').

I don't think automatic zero-filling makes too much sense the way it's presented right now. I'd like to see some more concrete use-cases that couldn't be better served by an alternative proposal.