Fix the really stupid way the user context is handled. #7

New Issue

torque · 2023-11-12T12:45:31-07:00

torque commented

2023-11-12 12:45:31 -07:00

This field should be stored as-is on the generic Parser object rather than cast to *anyopaque and stored on the type erased ParserInterface object. This would resolve the weird type mangling logic (u32 user context has to be provided as *u32) and some other contortions that are currently required.

Actually, thinking about it a bit more, this could be combined with #2 to have a unified mechanism. If the user needs a context object, they can just have it as a field on their custom output struct. The downside of that is that it would make passing a shared context to all of the commands in a hierarchy more difficult. The fields in the user Output object for the root of the tree would have to be the union of all the fields of each of the branches and leaves, which seems undesirable, as earlier stages of the pipeline may have undefined fields, which could definitely be a refactoring footgun. This seems like a big enough downside that I think it's the wrong direction to go.

We could improve the ergonomics around void contexts by allowing a broader range of callback function signatures (like omitting passing the context object if it is void). This would move a little bit more work out of the compiler and into the library, but _: void in every user callback looks ugly and is annoying to type, and in my own use so far this seems to be a pretty common use case.

This field should be stored as-is on the generic `Parser` object rather than cast to `*anyopaque` and stored on the type erased `ParserInterface` object. This would resolve the weird type mangling logic (`u32` user context has to be provided as `*u32`) and some other contortions that are currently required. Actually, thinking about it a bit more, this could be combined with #2 to have a unified mechanism. If the user needs a context object, they can just have it as a field on their custom output struct. The downside of that is that it would make passing a shared context to all of the commands in a hierarchy more difficult. The fields in the user `Output` object for the root of the tree would have to be the union of all the fields of each of the branches and leaves, which seems undesirable, as earlier stages of the pipeline may have undefined fields, which could definitely be a refactoring footgun. This seems like a big enough downside that I think it's the wrong direction to go. We could improve the ergonomics around `void` contexts by allowing a broader range of callback function signatures (like omitting passing the context object if it is void). This would move a little bit more work out of the compiler and into the library, but `_: void` in every user callback looks ugly and is annoying to type, and in my own use so far this seems to be a pretty common use case.

torque added this to the 0.1.0 milestone 2023-11-12 12:45:31 -07:00

torque added the

Kind/Enhancement

label 2023-11-12 12:45:31 -07:00

torque self-assigned this 2023-11-12 12:45:31 -07:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: factotum/NOCLIP#7