-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Move libfsm/start.c to Rust #344
Open
federicomenaquintero
wants to merge
21
commits into
katef:main
Choose a base branch
from
federicomenaquintero:rustify-fsm-start
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Move libfsm/start.c to Rust #344
federicomenaquintero
wants to merge
21
commits into
katef:main
from
federicomenaquintero:rustify-fsm-start
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a shame, but gets in the way of porting to Rust.
… the partially linked .o files. This hopefully avoids an ld bug where `-undefined dynamic_lookup` doesn't work on MacOS, and also removes the need for programs to link both the C and Rust libraries directly.
C bitfields in Rust are painful, and I'd like something that has the same ABI as Rust's bool. stdbool.h's bool is just the thing. In both struct fsm_state and struct fsm: * All the bitfields are packed together. * The field that follows the bitfields is a pointer, so it has pointer alignment, so the overall size of the structs shouldn't change.
These come from internal.h. For now the Rust types have the same ABI as the C types; we'll have parallel structs for a while until all the internals get rustified.
The strategy for this commit: * Implement clear_start / set_start / get_start in Rust as methods, with argument checks as assertions. * Implement a C ABI wrapper to match the fsm_clearstart() / etc. functions. The non-null argument checks go there. * Test both sets of functions right there. We cannot construct a Fsm in Rust just yet, so the tests hack up just enough initial state for the code to run; this can be removed later. * Remove start.c. Later we can move all of the C API to a c_api.rs or whatever.
I really don't know the best way to do this with pmake; someone who knows better should fix this :)
Another easy function, wheeee!
Instead of duplicating the code.
…loc block In all places where a state's .has_capture_actions gets accessed, there is an assert that the index of the state fits within the fsm->statecount. Since in the previous commit we initialize the has_capture_actions field when a new state is added, there is no need to initialize that field for all the as-yet-unused state structs in the realloc block.
The -1 was meant to be an OOM sentinel, but there is no place in the code that actually sets fsm->statecount to -1 when the states array cannot be (re)allocated. The code after the patch does the realloc(), and properly returns a failure code if realloc() fails. I don't think we need to preserve setting "errno = ENOMEM" since there is no other place in the code that signals OOM through errno.
…whole statealloc Only the first statecount state structs are valid, anyway.
The last commits I've pushed ( Those commits are useful outside of a Rust port. Please tell me if you'd rather have me submit a separate pull request for them, or if you'd prefer to cherry-pick them or whatever. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This depends on #343.
First we replace the bitfields in a couple of structs with
stdbool.h
, so we can have the same ABI as Rust'sbool
type.Then we make parallel structs in Rust for
struct fsm_state
andstruct fsm
. We'll keep these in sync with the C structs while the internals get rustified.Then we port
fsm_clearstart
,fsm_getstart
,fsm_setstart
to Rust, and removestart.c
.I probably totally botched the Makefiles to add
-lpthread -ldl
, needed by Rust libraries, but I need some help from someone who knows pmake :)