Don't require Plugin to impl Default #154
Conversation
Boscop
force-pushed
the
plugin-instance-new
branch
from
0da6e71 to
b793e8d
Compare
askeksa
left a comment
askeksa
left a comment
There was a problem hiding this comment.
Thanks a lot for doing this! This is a good improvement to the API.
| } | ||
| } | ||
| } | ||
| impl Plugin for LadderFilter { |
| /// # use vst::event::{Event, MidiEvent}; | ||
| /// # struct ExamplePlugin { host: HostCallback, send_buf: SendEventBuffer } | ||
| /// # impl Plugin for ExamplePlugin { | ||
| /// # fn new(host: HostCallback) -> Self { Self { host, send_buf: Default::default() } } |
There was a problem hiding this comment.
Why two levels of Self (here and below)?
There was a problem hiding this comment.
The first Self is just the return type.
The second one is the literal constructor.
| `src/lib.rs` | ||
|
|
||
| ```rust | ||
| #[macro_use] |
There was a problem hiding this comment.
Also fix the comment above to not mention Default.
| /// // ... | ||
| /// # extern crate vst; | ||
| /// # #[macro_use] extern crate log; | ||
| /// # use vst::plugin::{Plugin, Info}; |
There was a problem hiding this comment.
Fix the Plugin trait comment
/// All methods except `get_info` provide a default implementation which does nothing and can be
/// safely overridden.
to say that new is also a required method.
src/plugin.rs
Outdated
| /// // the host during initialization via Vst::new() | ||
| /// // the host during initialization in `Plugin::new()` | ||
| /// let host: HostCallback = Default::default(); | ||
| /// let version = host.vst_version(); |
There was a problem hiding this comment.
I'd suggest removing most of this example, since implementing Default for a Plugin is no longer needed and also not the recommended way of doing things.
Until we remove the Default implementation for HostCallback (if we decide to do that), the description could say something like
/// All methods in this struct will panic if the `HostCallback` was constructed using
/// `Default::default()` rather than being passed in via `Plugin::new`.
and then just have the two lines here and nothing else as the example.
|
@askeksa Thanks for the review. I applied the suggested changes in the new commit that I just pushed. |
|
@askeksa Everything ok now? Can I merge? |
|
Yes, looks good. |
Boscop
force-pushed
the
plugin-instance-new
branch
from
863496c to
2d9af1a
Compare
2 participants
fixes #153 by implementing
Plugin::newforPluginInstanceby usingunreachable!().It's safe because it's never called anyway.
(
Plugin::newis only called on the client side andPluginInstanceis only used on the host side):https://github.com/Boscop/rust-vst/blob/0da6e719a26ba67d8302caf26eda1539ee221e81/src/host.rs#L570