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

update attribute not working on internal struct? #369

Open
yanshay opened this issue Oct 27, 2023 · 3 comments
Open

update attribute not working on internal struct? #369

yanshay opened this issue Oct 27, 2023 · 3 comments
Labels
question Further information is requested

Comments

@yanshay
Copy link

yanshay commented Oct 27, 2023

I'm trying to use update, and in my tests it doesn't run on internal structs, it works only on top level. Is there a way to get it working also on internal structs? I can manually run it on the internal field but it means I need to know the implementation details while I'd like all updates to propagate through the container structure.

Here is my test code:

use deku::prelude::*;
use std::convert::TryInto;

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct IpAddr {
    #[deku(update = "9")]
    a: u8,
    b: u8,
    c: u8,
    d: u8,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct Container {
    ip_addr: IpAddr,
}

pub fn main() {
    let mut onlyip = IpAddr {
        a: 1,
        b: 2,
        c: 3,
        d: 4,
    };
    onlyip.update().unwrap();
    let onlyip_buf: Vec<u8> = onlyip.try_into().unwrap();
    println!("{:?}", onlyip_buf);

    let mut container = Container {
        ip_addr: IpAddr {
            a: 1,
            b: 2,
            c: 3,
            d: 4,
        },
    };
    container.update().unwrap();
    let container_buf: Vec<u8> = container.try_into().unwrap();
    println!("{:?}", container_buf);
}

This is the output it generated:

[9, 2, 3, 4]
[1, 2, 3, 4]

As seen, the update doesn't run in the second case

@wcampbell0x2a
Copy link
Collaborator

Recursive update seems like a good thing to add. Anyway, here is the "requires Clone + Copy and is really just a hack" code you can use:

#[derive(Debug, PartialEq, DekuRead, DekuWrite, Clone, Copy)]
struct IpAddr {
    #[deku(update = "9")]
    a: u8,
    b: u8,
    c: u8,
    d: u8,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct Container {
    #[deku(update = "{ let mut a = self.ip_addr; a.update()?; a }")]
    ip_addr: IpAddr,
}

@yanshay
Copy link
Author

yanshay commented Oct 27, 2023

This works for this case, however I also have a Vec in the relevant structure, and for this to work it is required for IpAddr implement Clone and Copy, but Vec doesn't implement Copy, so it can't work. Is there a way to bypass it?

What I did was to simply implement my own update function on Container that calls the IpAddr update. Not sure why the update through Deku requires IpAddr to implement Copy when my own function doesn't (maybe because it accepts &mut self)

@wcampbell0x2a
Copy link
Collaborator

deku emits the following for updating, for my example:

impl DekuUpdate for Container {
    fn update(&mut self) -> core::result::Result<(), ::deku::DekuError> {
        use core::convert::TryInto;
        self
            .ip_addr = ({
            let mut a = self.ip_addr;
            a.update()?;
            a
        })
            .try_into()?;
        Ok(())
    }
}

There isn't a reason that deku couldn't in the future support emitting this:

impl DekuUpdate for Container {
    fn update(&mut self) -> core::result::Result<(), ::deku::DekuError> {
        self.ip_addr.update()?;
        Ok(())
    }
}

Maybe allow an empty update for this behavior? I'll admit I don't use the update attribute much.

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct Container {
    #[deku(update)]
    ip_addr: IpAddr,
}

@wcampbell0x2a wcampbell0x2a added the question Further information is requested label Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants