Skip to content
This repository has been archived by the owner on Sep 18, 2020. It is now read-only.

Consider using enum_dispatch instead of dynamic dispatching #4

Open
marioortizmanero opened this issue Jul 20, 2020 · 0 comments
Open
Labels
consideration Has to be discussed before its implementation enhancement New feature or request

Comments

@marioortizmanero
Copy link
Member

marioortizmanero commented Jul 20, 2020

There's an important and repetitive pattern in the api, player and lyrics modules. These support multiple possible implementations of a base trait, and are associated in an enum. This requires a init_ function to initialize the struct depending on the chosen enum variant:

enum Things {
    ThingA,
    ThingB,
}

trait ThingBase {
    fn new() -> Self where Self: Sized;
    ...
}

// This would go in a different file
struct ThingA { ... }
impl ThingBase for ThingA { ... }

// This would go in a different file
struct ThingB { ... }
impl ThingBase for ThingB { ... }

pub fn init_thing(which: Things) -> Box<dyn ThingBase> {
    match which {
        Things::ThingA => ThingA::new(),
        Things::ThingB => ThingB::new(),
    }
}

This makes the implementations of ThingBase modular, as they can be split into different files, and is the most straightforward solution I found, which is what was used in the Python implementation of Vidify, too.

The second approach which will later be simplified by the enum_dispatch crate allows to use this solution instead, which avoids dynamic dispatching:

#[enum_dispatch]
enum Things {
    ThingA,
    ThingB,
}

#[enum_dispatch(Things)]
trait ThingBase {
    fn new() -> Self where Self: Sized;
    ...
}

// This would go in a different file
struct ThingA { ... }
impl ThingBase for ThingA { ... }

// This would go in a different file
struct ThingB { ... }
impl ThingBase for ThingB { ... }

// Can be initialized with `let thing: Things = ThingA::new().into()`

I haven't looked at it in depth but it would be an interesting change to avoid dynamic dispatching both to simplify the code (no Box<dyn Trait>), and to optimize it a bit (this shouldn't really be noticeable anyway, but the enum_dispatch claims to be ten times faster). What I'm not sure of is if the initialization of the structs would still be as easy directly from the enum. Avoiding init_api would reduce boilerplate as well, but it might not be exactly what I'm looking for. This might also add complexity to an important part of the core member, which must be relatively simple in case the GUI is implemented in a different language.

@marioortizmanero marioortizmanero added enhancement New feature or request consideration Has to be discussed before its implementation labels Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
consideration Has to be discussed before its implementation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant