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

rust: a question mark operator is a conditional exit #1113

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

samueltardieu
Copy link
Contributor

A question mark operator ? may exit the current function the same way a return
will. It must be counted in the "nexits" statistics.

A question mark operator `?` may exit the current function the same way a `return`
will. It must be counted in the "nexits" statistics.
@samueltardieu
Copy link
Contributor Author

Is there something missing for this PR to be reviewed and merged?

Copy link
Collaborator

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@calixteman calixteman merged commit b035001 into mozilla:master Sep 13, 2024
1 check failed
@alexle0nte
Copy link
Contributor

While working on #1091, I noticed that some tests are failing because all nodes corresponding to a Rust::QMARK are mistakenly counted as exit points.

For example, in this code:

pub fn assert_ser_tokens<T: ?Sized>(value: &T, tokens: &[Token])
where
    T: Serialize,
{
    let mut ser = Serializer::new(tokens);
    match value.serialize(&mut ser) {
        Ok(_) => {}
        Err(err) => panic!("value failed to serialize: {}", err),
    }

    if ser.remaining() > 0 {
        panic!("{} remaining tokens", ser.remaining());
    }
}

the ? on the first line, inside <T: ?Sized>, should not be counted as an exit point.
Therefore, adding Rust::QMARK in an OR condition with Rust::ReturnExpression doesn't fully address the issue.

@calixteman
Copy link
Collaborator

@alexle0nte could you file a bug please ?
@samueltardieu would you mind to have a look on it ?

Thank you.

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.

3 participants