search

jefffhaynes / BinarySerializer

A declarative serialization framework for controlling formatting of data at the byte and bit level using field bindings, converters, and code.
MIT License
292 stars 62 forks source link

Proposal: Allow applying FieldEndianness to classes/structs #96

Open BADF00D opened 6 years ago

BADF00D commented 6 years ago

I just reviewer a code of my colleague, that uses BinarySerializer. I noticed, that you have to apply the FieldEndiannessAttribute to each field individually, rather then applying it to the class or struct. I think it's very uncommon, that the endianess switches within a structure. So what do you think, if you can apply the attribute at class level?

So rather then writing this:

public class SomeData {
    [FieldEndianness(Endianness.Little)]
    [FieldOrder(0)]
    public ushort Id;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(1)]
    public short Order;
}

I want to write this:

[FieldEndianness(Endianness.Little)]
public class SomeData {
    [FieldOrder(0)]
    public ushort Id;

    [FieldOrder(1)]
    public short Order;
}

In rare cases, where endianess swaps, the attribute nearest to the field, wins:

[FieldEndianness(Endianness.Little)]
public class SomeData {
    [FieldOrder(0)]
    public ushort Id;

    [FieldOrder(1)]
    [FieldEndianness(Endianness.Big)]
    public short Order;
}
jefffhaynes commented 6 years ago

Have you tried it? I would think that it should work. If not, it’s certainly a bug!

— If you want to build a ship, don't drum up people to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea.

Antoine de Saint-Exupery

From: BADF00D notifications@github.com Sent: Monday, February 26, 2018 9:04:56 AM To: jefffhaynes/BinarySerializer Cc: Subscribed Subject: [jefffhaynes/BinarySerializer] Proposal: Allow applying FieldEndianness to classes/structs (#96)

I just reviewer a code of my colleague, that uses BinarySerializer. I noticed, that you have to apply the FieldEndiannessAttribute to each field individually, rather then applying it to the class or struct. I think it's very uncommon, that the endianess switches within a structure. So what do you think, if you can apply the attribute at class level?

So rather then writing this:

public class SomeData { [FieldEndianness(Endianness.Little)] [FieldOrder(0)] public ushort Id;

[FieldEndianness(Endianness.Little)]
[FieldOrder(1)]
public short Order;

}

I want to write this:

[FieldEndianness(Endianness.Little)] public class SomeData { [FieldOrder(0)] public ushort Id;

[FieldOrder(1)]
public short Order;

}

In rare cases, where endianess swaps, the attribute nearest to the field, wins:

[FieldEndianness(Endianness.Little)] public class SomeData { [FieldOrder(0)] public ushort Id;

[FieldOrder(1)]
[FieldEndianness(Endianness.Big)]
public short Order;

}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/jefffhaynes/BinarySerializer/issues/96, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AJSKR49GJGmOcItbwbjiwqkkKr_E2TSzks5tYroIgaJpZM4STSJN.

BADF00D commented 6 years ago

I tried an older version, but before I wrote this issue, I looked up the current implementation:

/// <summary>
    ///     Specifies the endianness (value byte order) for a field.
    /// </summary>
    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = true)]
    public sealed class FieldEndiannessAttribute : FieldBindingBaseAttribute, IConstAttribute

So the attribute can't be applied to classes and structs, because AttributeTargets.Class and AttributeTargets.Struct is missing. Or did I miss something?

jefffhaynes commented 6 years ago

No, that’s weird. I’ll take a look, thanks.

— If you want to build a ship, don't drum up people to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea.

Antoine de Saint-Exupery

From: BADF00D notifications@github.com Sent: Monday, February 26, 2018 10:14:08 AM To: jefffhaynes/BinarySerializer Cc: Jeff Haynes; Comment Subject: Re: [jefffhaynes/BinarySerializer] Proposal: Allow applying FieldEndianness to classes/structs (#96)

I tried an older version, but before I wrote this issue, I looked up the current implementation:

///

/// Specifies the endianness (value byte order) for a field. /// [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = true)] public sealed class FieldEndiannessAttribute : FieldBindingBaseAttribute, IConstAttribute

So the attribute can't be applied to classes and structs, because AttributeTargets.Class and AttributeTargets.Struct is missing. Or did I miss something?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/jefffhaynes/BinarySerializer/issues/96#issuecomment-368535344, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AJSKRzLbzXMgSm66hR_nEEr8kd0fxFneks5tYso_gaJpZM4STSJN.

jefffhaynes commented 6 years ago

Sorry, I was reading this on my phone and I completely misunderstood what you were asking. Currently nothing in binary serializer is applied in terms of type attributes and I'm not totally sure that would be an advantage. Can your colleague apply the attribute to the appropriate field in the containing parent object and would that not accomplish the same thing? Or alternatively, endianness can be passed in during serialization/deserialization.

The reason I like this approach is it allows you to reuse types with different attributes without having to redeclare them. As a concrete example, the ISO9660 spec has structures appearing over and over with alternating endiannesses.

Anyway, I'm not saying one way is right or wrong but adding support for type-level declarations would be no small thing. I'll think about it some more but I'd be curious to hear more about your specific use case and why you think it would be the better approach.

Thanks

BADF00D commented 6 years ago

We read data send from medical devices via RS232/USB/LAN. Usually the manufacturers of the device defines the endianess once per protocol. In rare cases this is switching during protocol extensions. So we can use your approach and apply this setting during deserialization. But if we use this approach, a type would no longer describe itself, because the endianess is declared somewhere else.

Our goals can be summarized as:

My colleague mentioned, that the endianess attribute distracts the reader from the information that is really interesting at this point: the field offset.

Imaging a class with 10 fields. 

public class SomeData {
    [FieldEndianness(Endianness.Little)]
    [FieldOrder(0)]
    public ushort Field0;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(1)]
    public ushort Field1;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(2)]
    public ushort Field2;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(3)]
    public ushort Field3

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(4)]
    public ushort Field4;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(5)]
    public ushort Field5;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(6)]
    public ushort Field6;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(7)]
    public ushort Field7;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(8)]
    public ushort Field8;

    [FieldEndianness(Endianness.Little)]
    [FieldOrder(9)]
    public ushort Field9;
}

In our case, the endianess does not change so the code could be simplified.

[FieldEndianness(Endianness.Little)]
public class SomeData {

    [FieldOrder(0)]
    public ushort Field0;

    [FieldOrder(1)]
    public ushort Field1;

    [FieldOrder(2)]
    public ushort Field2;

    [FieldOrder(3)]
    public ushort Field3

    [FieldOrder(4)]
    public ushort Field4;

    [FieldOrder(5)]
    public ushort Field5;

    [FieldOrder(6)]
    public ushort Field6;

    [FieldOrder(7)]
    public ushort Field7;

    [FieldOrder(8)]
    public ushort Field8;

    [FieldOrder(9)]
    public ushort Field9;
}

But in the end, its a convenience feature. I have no hard facts, for or again this approach, it was just an idea. Besides the work needed to implement this, can you imaging any disadvantage? Do you think it can harm the readability in some circumstances?

jefffhaynes commented 6 years ago

Right, I definitely see what you’re saying but why not simply add a parent class like this

public class Container
{
   [FieldEndianness(Endianness.Big)]
   public SomeData Data { get; set; }
}

Doesn’t that accomplish the same thing?

And to be clear, I don't think it's a bad idea, but it would add complexity to the framework. I will think about it but it raises issues of conflicting attribute resolution, etc.

BADF00D commented 6 years ago

Your approach would work, but SomeClass would not be self describing any more. With the current implementation, we prefer declaring declare the attribute at the field.

What do you mean with "raises issues of conflicting attribute resolution"? Do you think it might not be clear, which Attribute (at class or field level) has precedence over each other? I think it could be something like: the nearer the attribute it to the definition, the more precedence it takes.

E.g.:

public class Container
{
   [FieldEndianness(Endianness.Big)] //priority 1
   public SomeData Data { get; set; }
}
[FieldEndianness(Endianness.Little)] //priority 2
public class SomeData {

    [FieldOrder(0)]
    public ushort Field0;

    [FieldOrder(1)]
    [FieldEndianness(Endianness.Big)] //prority 3 - highest
    public ushort Field1;
...
}
jefffhaynes commented 6 years ago

I think the type definition would have to be the default configuration and anything specified in the object graph would be considered overriding.

Anyway, I’ll play around with it and see what it would take.

I’m still not completely convinced that tightly coupling the type definition to what would essentially be the architecture (endianness) context makes sense but maybe I’m thinking about it wrong.

— If you want to build a ship, don't drum up people to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea.

Antoine de Saint-Exupery

From: BADF00D notifications@github.com Sent: Tuesday, February 27, 2018 7:36:53 AM To: jefffhaynes/BinarySerializer Cc: Jeff Haynes; Comment Subject: Re: [jefffhaynes/BinarySerializer] Proposal: Allow applying FieldEndianness to classes/structs (#96)

Your approach would work, but SomeClass would not be self describing any more. With the current implementation, we prefer declaring declare the attribute at the field.

What do you mean with "raises issues of conflicting attribute resolution"? Do you think it might not be clear, which Attribute (at class or field level) has precedence over each other? I think it could be something like: the nearer the attribute it to the definition, the more precedence it takes.

E.g.:

public class Container { [FieldEndianness(Endianness.Big)] //priority 1 public SomeData Data { get; set; } } [FieldEndianness(Endianness.Little)] //priority 2 public class SomeData {

[FieldOrder(0)]
public ushort Field0;

[FieldOrder(1)]
[FieldEndianness(Endianness.Big)] //prority 3 - highest
public ushort Field1;

... }

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/jefffhaynes/BinarySerializer/issues/96#issuecomment-368859855, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AJSKR-iOUQnQR86yHfX0Jkkz1pTRqviVks5tY_bkgaJpZM4STSJN.

BADF00D commented 6 years ago

Thanks for thinking about it.

Btw: Great work!

jefffhaynes commented 6 years ago

Thank you, I’m glad it’s useful!

— If you want to build a ship, don't drum up people to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea.

Antoine de Saint-Exupery

From: BADF00D notifications@github.com Sent: Tuesday, February 27, 2018 8:13:02 AM To: jefffhaynes/BinarySerializer Cc: Jeff Haynes; Comment Subject: Re: [jefffhaynes/BinarySerializer] Proposal: Allow applying FieldEndianness to classes/structs (#96)

Thanks for thinking about it.

Btw: Great work!

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/jefffhaynes/BinarySerializer/issues/96#issuecomment-368868729, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AJSKR81kRczv2778BkIUwIALvMVSa8x-ks5tY_9egaJpZM4STSJN.

ghost commented 6 years ago

@jefffhaynes If I haven't said it yet: Thank you for your work! Your library is really helpful.

jefffhaynes commented 6 years ago

Great, thank you!

— If you want to build a ship, don't drum up people to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea.

Antoine de Saint-Exupery

From: Daniel Müller notifications@github.com Sent: Tuesday, February 27, 2018 9:43:04 AM To: jefffhaynes/BinarySerializer Cc: Jeff Haynes; Mention Subject: Re: [jefffhaynes/BinarySerializer] Proposal: Allow applying FieldEndianness to classes/structs (#96)

@jefffhayneshttps://github.com/jefffhaynes If I haven't said it yet: Thank you for your work! Your library is really helpful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/jefffhaynes/BinarySerializer/issues/96#issuecomment-368900302, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AJSKR4VDQxVkZwfpjN10hiI578Rt9Nd2ks5tZBRtgaJpZM4STSJN.