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

WIP: First implementation for LV2 Extensions system #12

Merged
merged 4 commits into from Jul 15, 2019
Merged

Conversation

@prokopyl
Copy link
Contributor

prokopyl commented May 19, 2019

Implementation is functional, but can maybe be cleaned up a bit, and still needs documentation!

@prokopyl prokopyl requested a review from Janonard May 19, 2019
@prokopyl prokopyl self-assigned this May 19, 2019
Copy link
Member

Janonard left a comment

I'm fine with the general ideas of that PR, but I've got some points that I think could be done better. Please take a look:

core/src/extension.rs Show resolved Hide resolved
core/src/extension.rs Show resolved Hide resolved
core/src/plugin/mod.rs Outdated Show resolved Hide resolved
core/src/plugin/mod.rs Show resolved Hide resolved

unsafe extern "C" fn foo_ext_impl<P: FooExtension>(handle: *mut c_void) -> f32 {
(&*(handle as *const P)).foo()
}

This comment has been minimized.

Copy link
@Janonard

Janonard Jun 8, 2019

Member

This should be moved into the extension trait, hidden from the docs: From a semantic point of view, it is a trait method that is implemented for all types that implement the trait, but it should not be seen or used by the user. Therefore, it should be #[doc(hidden)].

This comment has been minimized.

Copy link
@prokopyl

prokopyl Jun 9, 2019

Author Contributor

This function should be already private in a module implementing an LV2 extension, so no #[doc(hidden)] is needed here, although I agree that the this isn't clear at all for this test.

I reorganized the contents of this test to create two example modules (which would be crates implementing fictional LV2 specifications): lv2_foo and lv2_bar. The only public things from these modules are the extension trait, and the interface struct in their -sys crate.
I hope this makes things clearer about what is accessible to the end-user, and what is not. 🙂

This comment has been minimized.

Copy link
@Janonard

Janonard Jun 10, 2019

Member

I still think that this method should be inside the trait, for example because we would need one generic parameter less. It would also be easier to identify a safe trait method with it's unsafe counter-part.

This comment has been minimized.

Copy link
@prokopyl

prokopyl Jun 10, 2019

Author Contributor

The issue with putting it in the trait is that it makes it callable by the end user (which should not happen, as it is a callback for the host to call), and worse, it makes it override-able by the user, which is not something that is wanted by the implementer of the extension trait, as this function is the one that takes care of setting up all the necessary abstractions, and changing it is pretty much guaranteed to be incorrect.

This function (foo_ext_impl) is not an unsafe counterpart of the function to be implemented in the trait, it is the callback given to the host that will handle parameter conversion, unwind catching, error management, etc. It should not be accessible to the end-user by any means.

This comment has been minimized.

Copy link
@Janonard

Janonard Jun 12, 2019

Member

This callback method is unsafe, if it is placed in the trait or not, and if we put it in the trait, it will also be hidden from the docs. This means, that the users have to a) discover that this callback exists, which they can only do if they read the source code, and b) write unsafe code to call or override this method, which tells them that they have to know what they are doing. If the users know what they are doing, they know that they should not call or override this method, because the (hidden) documentation in the source code tells them so. But if they do override the callback, which damage could be done? There will always be a callback function and we will therefore never have an invalid callback function pointer. The only thing I can think of is misuse of the raw pointers, but since the users are writing unsafe code, we have to trust them that they do proper work; That's one meaning of unsafe code: You always have to trust that unsafe code is correct.

On the other hand, as I said before, placing this method in the trait makes the extension code easier to read, because you know which callback belongs to which method and you have no type parameters. From my point of view, you want to hide too much from the users, which I disagree with: These kind of things should be exposed as unsafe code, which means that it is "hidden" to the normal user, but exposed for those who need that extra bit of control.

This comment has been minimized.

Copy link
@prokopyl

prokopyl Jun 13, 2019

Author Contributor

I fail to see the use case for overriding the "entry point" host callback, as it basically overwrites the whole abstraction done by the implementer of the Extension (they would need to completely rewrite the whole parameter conversion, panic catching, thread-related invariants, etc.), basically monkey-patching a core function like this feels very hacky to me. But I see your point in general, extra control may be needed at times.

However, I think "how much should the extension expose to the user" is a question only the implementer of said extension should ask themselves, not us (as of making the extension system as a whole). The current system allows the callback to be either inside or outside of the trait, and if they keep it outside they can choose to expose any selection of (safe or unsafe) details to the user by adding that many methods on the trait for the end user, so i can see both points of view cohabiting in the ecosystem.

And if the end user disagrees with the implementer (and don't want to submit a bug report/PR for some reason), they can also write the extension in the way they want. After all, you can import as many implementations of a same extension (or port, or feature) as you want, as long as you only use one for each plugin. 🙂

As for this specific case, I don't think Foo or Bar don't need extra control as they are just tests. 😉 However, I will make a page explaining how to implement an extension, and what are the different ways you can expose more or less stuff to the user.

(Also, I really dislike exposing unsafe mehods with #[doc(hidden)]. If they are in the trait, they are publicly visible, so they should be properly documented. Otherwise it would only cause confusion to users as they could see it in the code but not access it in the docs.)

This comment has been minimized.

Copy link
@Janonard

Janonard Jun 14, 2019

Member

I also see no use case for it, but I also don't see that it could do any harm that regular unsafe doesn't do. The mean reason I keep discussing is that the style of this example/test will heavily influence upcoming extensions, which we will have to implement too! Therefore, this example/test has to follow a style that is suitable for upcoming, bigger extensions and I think that keeping the callback functions separate from the trait will make the code messy.

prokopyl added 3 commits May 19, 2019
@prokopyl prokopyl force-pushed the extensions branch from a633f9f to 9a154dc Jun 9, 2019
@prokopyl prokopyl requested a review from Janonard Jun 9, 2019
core/src/extension.rs Outdated Show resolved Hide resolved
Copy link
Member

Janonard left a comment

I've also added all of the other changes! 🙂

core/tests/extension.rs Outdated Show resolved Hide resolved
core/tests/extension.rs Outdated Show resolved Hide resolved
core/tests/extension.rs Outdated Show resolved Hide resolved
core/tests/extension.rs Outdated Show resolved Hide resolved
core/tests/extension.rs Outdated Show resolved Hide resolved
core/tests/extension.rs Outdated Show resolved Hide resolved
core/tests/extension.rs Outdated Show resolved Hide resolved
Co-Authored-By: Jan-Oliver Opdenhövel <janonard@protonmail.com>
@Janonard Janonard merged commit 098506a into master Jul 15, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Janonard

This comment has been minimized.

Copy link
Member

Janonard commented Jul 15, 2019

I want to continue the work on the project and since this discussion is frozen and only about a minor detail, it's okay to merge it now.

@Janonard Janonard deleted the extensions branch Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.