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

Pass reference values in guards and owned values in actions #49

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

oblique
Copy link
Contributor

@oblique oblique commented Oct 27, 2022

Closes #48

@oblique
Copy link
Contributor Author

oblique commented Oct 27, 2022

Let me know if you have any comments

Copy link
Collaborator

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

The changes seem reasonable to me. This will definitely have some impact on fallible actions support though discussed in #44, but I don't think that should block a change like this. There's definitely value in simply moving data instead of using references and letting the compiler be smart about copying/cloning as appropriate.

@oblique
Copy link
Contributor Author

oblique commented Oct 28, 2022

Pushed new changes. I avoided panicking, however fn state still needs unwrap, so I used unwrap_unchecked instead, but I don't like that it is unsafe.

If you have any other ideas let me know.

@oblique
Copy link
Contributor Author

oblique commented Oct 29, 2022

I had an idea today to introduce Error::Poisoned. Normally this error should never happen, but it's the only way to avoid unwrap without unsafe.

These are the reasons that this error will happen:

  • smlang did not restore the self.state to Some. For example before returning GuardFailed or InvalidEvent.
  • The user panicked in guard or action functions, so the self.state was not restored to Some.

This is the generated code:

    #[inline(always)]
    pub fn state(&self) -> Result<&States, Error> {
        self.state.as_ref().ok_or_else(|| Error::Poisoned)
    }

    pub fn process_event(&mut self, mut event: Events) -> Result<&States, Error> {
        match self.state.take().ok_or_else(|| Error::Poisoned)? {
            States::State1 => match event {
                Events::Event1(event_data) => {
                    match self.context.guard1(&event_data) {
                        Ok(()) => {
                            let _data = self.context.action1(event_data);
                            self.state = Some(States::State2(_data));
                        }
                        Err(e) => {
                            self.state = Some(States::State1);
                            return Err(Error::GuardFailed(e));
                        }
                    }
                    self.state()
                }
                _ => {
                    self.state = Some(States::State1);
                    Err(Error::InvalidEvent)
                }
            },
            States::State2(state_data) => match event {
                _ => {
                    self.state = Some(States::State2(state_data));
                    Err(Error::InvalidEvent)
                }
            },
            state => {
                self.state = Some(state);
                Err(Error::InvalidEvent)
            }
        }
    }

@oblique
Copy link
Contributor Author

oblique commented Nov 1, 2022

Rebased. Now the generated code is:

    pub fn state(&self) -> Result<&States, Error> {
        self.state.as_ref().ok_or_else(|| Error::Poisoned)
    }

    pub fn process_event(&mut self, mut event: Events) -> Result<&States, Error> {
        match self.state.take().ok_or_else(|| Error::Poisoned)? {
            States::State1 => match event {
                Events::Event1(event_data) => {
                    if let Err(e) = self.context.guard1(&event_data) {
                        self.state = Some(States::State1);
                        return Err(Error::GuardFailed(e));
                    }
                    let _data = self.context.action1(event_data);
                    self.state = Some(States::State2(_data));
                    self.state()
                }
                _ => {
                    self.state = Some(States::State1);
                    Err(Error::InvalidEvent)
                }
            },
            States::State2(state_data) => match event {
                _ => {
                    self.state = Some(States::State2(state_data));
                    Err(Error::InvalidEvent)
                }
            },
            state => {
                self.state = Some(state);
                Err(Error::InvalidEvent)
            }
        }
    }

@oblique oblique requested review from ryan-summers and andresv and removed request for ryan-summers and andresv November 1, 2022 19:23
Copy link
Collaborator

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

I'd like to let this kind of change rest for a bit before releasing it. There's some implications of moving state data that I'm not sure I've fully understood yet.

Copy link
Collaborator

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for persisting through my nagging :)

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.

Actions should consume data instead of using them as reference
3 participants