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

Implement Clone for CompileOptions #148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

simonask
Copy link

Motivation: shaderc::CompileOptions::clone() is actually kind of awkward and not particularly useful, because its return value has the inferred lifetime parameter '_ from the original object, rather than 'a, so the original object must be kept around anyway.

As far as I can tell, there is no reason that shaderc::CompileOptions could not implement the regular Clone trait, rather than its current special fn clone() -> Option<Self>.

This is a breaking change, because the signature of the Clone trait is different in the following ways:

  • It returns Self rather than Option<Self>. This seems more correct because shaderc_compile_options_clone() cannot fail except for heap allocation failure (it calls a nothrow trivial copy constructor), which is a degenerate scenario regardless.
  • The lifetime parameter on the return value is 'a rather than '_. This seems more correct, since the current clone() does not borrow from the existing object.

In addition, the inner include_callback_fn was changed from Box<dyn Fn(...)> to be Rc<dyn Fn(...)>. This should be fine because CompileOptions is already !Send due to its raw pointer member, and the function is Fn rather than FnMut.

Despite this being technically a breaking change, I think it might still be worth it. I hope you will agree. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant