Add runtime checks for builder position #436
Conversation
EricLBuehler
changed the title
src/builder.rs
Outdated
| #[derive(Error, Debug, PartialEq, Eq)] | ||
| pub enum BuilderError { | ||
| #[error("Builder position is not set")] | ||
| PositonError, |
There was a problem hiding this comment.
Maybe UnsetPosition would be a better name? It's sorta redundant to have Error in the variant name
There was a problem hiding this comment.
True. Changing that now - and I will also run typos over Inkwell. Perhaps adding typos can be added to CI?
There was a problem hiding this comment.
I didn't know about typos before - could you open a ticket please?
There was a problem hiding this comment.
Yes, will open it now.
|
Glad to help! |
| #[error("GEP error: does not point to a struct or index is out of bounds")] | ||
| GEPError, |
| #[error("GEP error: does not point to a struct or index is out of bounds")] | ||
| GEPError, |
There was a problem hiding this comment.
This is called GEPError, in my head it goes like "any GEP error goes into this variant", but there are actually 2 other GEP error variants.
I'd say this one should have a different name or be broken down into more variants.
WDYT? Is this worth an issue?
There was a problem hiding this comment.
I removed the GEPError on my fork. Turns out, GEPPointee and GEPIndex are all that were needed.
There was a problem hiding this comment.
Should I reopen another PR?
Edit: I will bundle these changes in my next PR regarding typos.
There was a problem hiding this comment.
Oh nice, I'd say yes! But we can wait for TheDan to be 100% sure.
Edit: I just saw your edit. 😆
I added an
assert_eqto check that the builder has a position set in everybuild_*method, which improves safety.Description
I added an
enumto keep track of the state, which was added to theBuilderstruct. The state on construction isNotSet, and theposition_*methods are used to set the state toSet. I factored out the rawassert_eqinto a macro to improve code readability.Related Issue
This resolves the problem discussed in #425.
How This Has Been Tested
I ran the tests locally on my WSL2 Ubuntu 22.04, as well as the Github Actions on my fork, all of which passed.
Checklist