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

Refactor parsing #113

Merged
merged 16 commits into from
Mar 5, 2024
Merged

Refactor parsing #113

merged 16 commits into from
Mar 5, 2024

Conversation

KillingSpark
Copy link
Owner

Parsing was updating the offset of the UnmarshalContext all over the place. This PR cleans this up by making offset and buf private and adding read_xxx functions for the primitives to the UnmarshalContext, which

  • aligns itself correctly
  • parses the primitive
  • updates its offset

This is a big refactor and the testing seems like it could use a good cleanup itself.

@MaxVerevkin
Copy link
Contributor

This breaks parameter parsing for org.freedesktop.DBus.Properties.Set, which fails with NotEnoughBytes. I didn't dig further yet.

@KillingSpark
Copy link
Owner Author

KillingSpark commented Feb 28, 2024

Yeah I wasn't confident that this is correct that's why I didn't push it to main immediately.

But I think the idea behind the refactor is good, I'll keep working on it.

Thanks for testing though!

@KillingSpark
Copy link
Owner Author

What kind of variant are you trying to parse into? The param based ones?

@MaxVerevkin
Copy link
Contributor

What kind of variant are you trying to parse into? The param based ones?

No, the one with the get<T: Unmarshal>() method. I will try to reduce it and submit a new test case.

@MaxVerevkin
Copy link
Contributor

#[test]
fn todo_name() {
    let mut m = MarshalledMessageBody::new();
    m.push_param("test.interface").unwrap();
    m.push_param("test_property").unwrap();
    m.push_param(crate::wire::marshal::traits::Variant(42u8))
        .unwrap();

    let mut parser = m.parser();
    assert_eq!(parser.get::<&str>().unwrap(), "test.interface");
    assert_eq!(parser.get::<&str>().unwrap(), "test_property");
    assert_eq!(
        parser
            .get::<crate::wire::unmarshal::traits::Variant>()
            .unwrap()
            .get::<u8>()
            .unwrap(),
        42
    );
}

@KillingSpark
Copy link
Owner Author

Thanks for the test case! The problem in the end was that the reported bytes used and the actual bytes used where out of whack because of the padding.

I think there is actually no need for the reporting of used bytes at all. If you do want that information you can get at it by looking at the remainders length before and after a call to Unmarshal::unmarshal. I think I will remove this.

@MaxVerevkin
Copy link
Contributor

I think there is actually no need for the reporting of used bytes at all. If you do want that information you can get at it by looking at the remainders length before and after a call to Unmarshal::unmarshal. I think I will remove this.

I also wondered why this was needed at all 😄

@KillingSpark
Copy link
Owner Author

I also wondered why this was needed at all 😄

The things you notice when you revisit code from a few years back :)

@KillingSpark KillingSpark merged commit 4bc3326 into master Mar 5, 2024
8 checks passed
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.

2 participants