diwic / dbus-rs

D-Bus binding for the Rust language
Other
591 stars 133 forks source link

Derive Macros #477

Closed ModProg closed 2 weeks ago

ModProg commented 3 months ago

What do you think about adding derive macros for e.g. Get, Append, ...

I did some experiments: macro source and usage example and would be willing to make a pr for them.

diwic commented 3 months ago

Hi @ModProg,

There is a draft PR here written by somebody else: https://github.com/diwic/dbus-rs/pull/459

Maybe you can collaborate on that one?

ModProg commented 3 months ago

I did not see that one, I'll have a look.

ModProg commented 3 months ago

@diwic, I'd probably rather take my own implementation and finish it up, unless you already agreed on the implementation/design of #459 and want to keep it.

One thing I'd personally rather do would be about 1 macro per trait, allowing you to derive one of them but manually implement others, as this is also the common practice in most of the rust ecosystem.

diwic commented 3 months ago

@ModProg Sure, I'm okay with you making a different implementation and trying to upstream it, since the other one is so stalled. It would still be a separate crate (not part of the main dbus crate) but it could be in the same repo, just as dbus-crossroads, dbus-tokio etc.

After upstreaming will you be around for a while? In case users discover bugs in the crate I'll ping you to have you handle them.

ModProg commented 3 months ago

yeah of course.

When you ping me I'll usually be around within a week.

ModProg commented 2 months ago

Would you prefer to use a LazyLock to avoid reevaluating the signature for e.g. structs? Or should this be unproblematic because signatures usually are quick to evaluate, this would avoid the allocation, but LazyLock isn't free either, so I'm unsure how much value there is:

use std::sync::LazyLock;
struct Tuple(String, u8);
impl Arg for Tuple {
    const ARG_TYPE: ArgType = ArgType::Struct;
    fn signature() -> Signature<'static> {
        let mut __signature = String::from("(");
        __signature.push_str(&<String as Arg>::signature());
        __signature.push_str(&<u8 as Arg>::signature());
        __signature.push(')');
        Signature::new(__signature).unwrap()
    }
    // vs
    fn signature() -> Signature<'static> {
        static __SIGNATURE: LazyLock<&'static str> = LazyLock::new(|| {
            let mut __signature = String::from("(");
            __signature.push_str(&<String as Arg>::signature());
            __signature.push_str(&<u8 as Arg>::signature());
            __signature.push_str(")\0");
            // static LazyLock would leak anyway, might as well make it explicit
            __signature.leak()
        });
        // SAFETY: \0 is appended
        unsafe { Signature::from_slice_unchecked(&__SIGNATURE) }
    }
}
diwic commented 2 months ago

@ModProg We could add a Signature::new_unchecked function if you're afraid of the cost of re-evaluation?

I don't think a Lazylock belongs in this context at all, as signature evaluation have nothing to do with thread synchronization.

ModProg commented 2 months ago

I don't think a Lazylock belongs in this context at all, as signature evaluation have nothing to do with thread synchronization.

LazyLock is just used instead of lazy_static! to ensure each signature is only created once, and then reused. It's LazyLock instead of LazyCell because LazyCell cannot be used in statics.

new_unchecked would only be half the story, because depending on how signature is implemented for the field types, those might call the regular Signature::new.

But as (A, B) implements it the "safe" way, I'll mirror that implementation for now, it'd be an easy change to adopt something more efficient later on.

On second thought, I don't think my idea of using a lazy is a valid pattern at all once generics come into play, because you cannot use generics in statics.

diwic commented 2 months ago

@ModProg Actually now that I think of it, it does make sense to do re-evaluation because there are limits; like maximum 32 levels of structs or something like that. So even if A and B are valid signatures it could be that (AB) is invalid.

diwic commented 2 weeks ago

Closing due to inactivity. Please reopen if or when you want to work on this again.