zeek / spicy

C++ parser generator for dissecting protocols & files.
https://docs.zeek.org/projects/spicy
Other
251 stars 37 forks source link

Allow to map bitfield fields into parent scope #1468

Closed J-Gras closed 1 year ago

J-Gras commented 1 year ago

It would be nice if fields of a bitfield can be mapped in the parent scope:

type Foo = unit {
  b: bitfield(8) {
    x: 0..3;
    y: 4..7;
  };
};

For example, not naming the bitfield could make x and y directly accessible from Foo.

See Slack for a workaround at the cost of performance.

bbannier commented 1 year ago

Without major rewrites any fix to this will likely only add syntactic sugar and incur the same runtime cost.

J-Gras commented 1 year ago

Given that the manual approach requires ~3x the lines of code, the syntactic sugar would still be helpful I think.

rsmmr commented 1 year ago

Mind elaborating why this would be so useful? Why is foo.x preferable over foo.b.x?

Background here is that Spicy actually only stores b inside the final struct: a single integer typed as uint8. x and y are computed from b on access by shifting accordingly. So there's no overhead really either way.

J-Gras commented 1 year ago

Mind elaborating why this would be so useful? Why is foo.x preferable over foo.b.x?

This is mostly for estethic reasons. In the use case this came up, the fields of the bitfield are defined as "first-class members", just like other fields of the protocol specification. Introducing the additional nesting (including meaningful naming) just feels a bit unnatural. Along those lines, I was also aiming to use the automatic export of types, which promises to save a lot of boilerplate code (now wondering whether bitfields are supported). So all in all, I am just trying to minimize work while coding closely along a protocol spec.

rsmmr commented 1 year ago

This is mostly for estethic reasons. In the use case this came up, the fields of the bitfield are defined as "first-class members", just like other fields of the protocol specification. Introducing the additional nesting (including meaningful naming) just feels a bit unnatural.

Ok, got it. Yeah, your suggestion of not naming the bitfield would work well then I think. Like:

type Foo = unit {
  : bitfield(8) {
    x: 0..3;
    y: 4..7;
  };
};

Then foo.x and foo.y would be allowed. Performance would remain the same: internally we'd just map the IDs accordingly during compilation.

(now wondering whether bitfields are supported)

Good question. :-)

bbannier commented 1 year ago

We should check what changes this needs for Zeek's Spicy integration.

rsmmr commented 1 year ago

Re-assigning to me, I'm adding this to the other bitfield work.