google / emboss

Emboss is a tool for generating code that reads and writes binary data structures.
Apache License 2.0
68 stars 21 forks source link

`requires` attribute on struct/bits has no effect #58

Closed acsaeed closed 1 year ago

acsaeed commented 1 year ago

This code compiles and runs with no issue:

*protocol.emb*

  struct Packet:
    [requires: number < another]
    0 [+4] UInt number
    $next [+4] UInt another

*main.cc*

  char buffer[64];
  auto view = test::MakePacketView(buffer, 64);
  view.number().Write(12);
  view.another().Write(6);

It's unclear whether the following behavior should be allowed to compile at all (as it was our attempt to enforce default values, an upcoming feature for Emboss), but it's worth noting that this also has no effect:

struct Packet(number_value: UInt:32):
  [requires: number == number_value]
  0 [+4] UInt number

The requires attribute on individual fields works as intended.

reventlov commented 1 year ago

This is WAI. In your first example, view.Ok() should return false at the end; methods on individual fields are unaffected.

The conceptual reasoning behind this is that when number < another is false, it is only the relationship between number and another that is invalid: it is not clear whether the fix is to increase another or decrease number.

It is also pretty easy to create requirements that cannot be met unless two or more fields are updated simultaneously, which would be awkward if Write() CHECK-failed:

struct Packet:
  [requires number == number2]
  0 [+4]  UInt  number
  4 [+8]  UInt  number2

In your second example, yes, that should compile. A parameter can be used in any place that a virtual field can be used. (Internally, they are virtual fields, just like let foo = fields.)

If you would like to file a feature request, one could imagine allowing:

struct Packet(number_value: UInt:32):
  0 [+4]  UInt  number
    [requires: this < number_value]

... which implies that number is the erroneous field. I haven't thought through all of the consequences, but there isn't a particularly deep reason that this isn't allowed, it's just that view.number() returns a UInt view that has no knowledge of its parent, which makes it difficult to reference sibling fields. (This is also why you have to reference constants with the name Type.constant instead of just constant.)

acsaeed commented 1 year ago

I see! Sorry for the misunderstanding. Thanks for taking the time to explain all this.