Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to map bitfield fields into parent scope #1468

Closed
Tracked by #1481
J-Gras opened this issue Jun 23, 2023 · 7 comments
Closed
Tracked by #1481

Allow to map bitfield fields into parent scope #1468

J-Gras opened this issue Jun 23, 2023 · 7 comments
Assignees
Labels
Enhancement Improvement of existing functionality

Comments

@J-Gras
Copy link
Contributor

J-Gras commented Jun 23, 2023

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 bbannier added the Enhancement Improvement of existing functionality label Jun 23, 2023
@bbannier
Copy link
Member

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

@J-Gras
Copy link
Contributor Author

J-Gras commented Jun 23, 2023

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

@rsmmr
Copy link
Member

rsmmr commented Jun 26, 2023

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
Copy link
Contributor Author

J-Gras commented Jun 26, 2023

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
Copy link
Member

rsmmr commented Jun 27, 2023

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
Copy link
Member

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

@bbannier bbannier self-assigned this Jun 29, 2023
@bbannier bbannier mentioned this issue Jul 6, 2023
9 tasks
@rsmmr
Copy link
Member

rsmmr commented Aug 4, 2023

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

@rsmmr rsmmr assigned rsmmr and unassigned bbannier and rsmmr Aug 4, 2023
rsmmr added a commit that referenced this issue Aug 4, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.
rsmmr added a commit that referenced this issue Aug 4, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.

TODO:
  - Add validation to catch ambigious access paths.
rsmmr added a commit that referenced this issue Aug 4, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.

TODO:
  - Extend RTTI to make field's `isAnonymous()` available.
  - Add validation to catch ambigious access paths.
rsmmr added a commit that referenced this issue Aug 7, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.

TODO:
  - Extend RTTI to make field's `isAnonymous()` available.
  - Add validation to catch ambigious access paths.
rsmmr added a commit that referenced this issue Aug 9, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.
rsmmr added a commit that referenced this issue Aug 11, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.
rsmmr added a commit that referenced this issue Aug 14, 2023
This works now:

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

      on %done {
        print self.x, self.y;
      }
};

Closes #1468.
@rsmmr rsmmr closed this as completed in f3a8bdf Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement of existing functionality
Projects
None yet
Development

No branches or pull requests

3 participants