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

print API: Box end_ids and end_id_count in a struct for callbacks. #497

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

silentbicycle
Copy link
Collaborator

Move the end_id array and its count into a struct for state metadata, and rename access throughout to end_ids and end_id_count.

Upcoming changes for eager output IDs will soon be passing more info to all of these callbacks, but only callers making use of those fields need to care. Instead of making callers add more (void) param; declarations all over the place to avoid warnings, just pass in a metadata struct pointer. Also, "count" is a pretty generic name and what it refers to will soon be ambiguous.

This should not be a functional change on its own, it's just clearing the way for new features.

Move the end_id array and its count into a struct for state metadata,
and rename access throughout to end_ids and end_id_count.

Upcoming changes for eager output IDs will soon be passing more info to
all of these callbacks, but only callers making use of those fields need
to care. Instead of making callers add more `(void) param;` declarations
all over the place to avoid warnings, just pass in a metadata struct
pointer. Also, "count" is a pretty generic name and what it refers to
will soon be ambiguous.

This should not be a functional change on its own.
The IR struct is about to get another id & count pair.

This loses storing the count as a 31-bit bitfield, but if the
goal for that is saving memory then the ids array allocation could
be replaced with a struct that contains the count, and then each
IR without endids will save more space than the current approach.
fsm_end_id_t *ids; /* NULL -> 0 */
size_t count:31; // :31 for packing
struct ir_state_endids {
fsm_end_id_t *ids; /* NULL -> 0 */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loses the :31 for packing, but if the goal is to save memory per IR state, then this could instead be a separately allocated struct like struct ir_state_endids { size_t count; fsm_end_ids_t ids[]; } and states without endids (which will be most states) save storing the count entirely. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, the packing isn't that important.

@silentbicycle
Copy link
Collaborator Author

I also moved it into a struct in the IR, because the same issues apply there -- it's about to get a different ID type (eager output IDs) and "ids" and "count" bare are ambiguous.

@silentbicycle
Copy link
Collaborator Author

This failed due to the fuzzer hitting an unrelated bug, for (?<m. It may not be exactly the same bug as #386 but probably closely related if not.

@katef
Copy link
Owner

katef commented Sep 27, 2024

I might come back and rename this, the names are all too verbose for me...

@katef katef merged commit 0961848 into main Sep 27, 2024
346 checks passed
@katef katef deleted the sv/move-codegen-callback-metadata-args-into-struct branch September 27, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants