Convert Context into an enum - #399
Conversation
This requires a bit more error handling than before, but some of this will be cleaned up once all data structures are converted to enums. Signed-off-by: Martin Robinson <martin@abandonedwig.info>
sagudev
left a comment
sagudev
left a comment
There was a problem hiding this comment.
comment(non-blocking): One thing that wgpu also has are traits: https://docs.rs/wgpu-hal/latest/wgpu_hal/trait.Api.html. Every type implements a trait (with it's types provided via associated types) and even dispatch type implements it (associated types are also dispatch types). I think that would allow us to simplify and unify more.
| descriptor: &ContextDescriptor, | ||
| share_with: Option<&Context>, | ||
| ) -> Result<Context, Error> { | ||
| let share_with = match share_with { |
There was a problem hiding this comment.
nit(non-blocking):
let share_with = share_with.map(|x| x.angle()).transpose()?;There was a problem hiding this comment.
Oh, cool! I've done this everywhere.
| let context: &mut AngleContext = context.try_into()?; | ||
| if context.egl_context == egl::NO_CONTEXT { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why moving the check? This can have bad implications.
There was a problem hiding this comment.
This was to avoid unpacking the AngleContext more than once (unbind_surface_from_context and destroy_surface both take a Context). I've moved this back to the origin order and now just unpack it more than once, the first time non-mutably.
| let context: &mut HardwareBufferContext = match context.try_into() { | ||
| Ok(context) => context, | ||
| Err(error) => return Err((error, surface)), | ||
| }; |
There was a problem hiding this comment.
nit(non-blocking):
| let context: &mut HardwareBufferContext = match context.try_into() { | |
| Ok(context) => context, | |
| Err(error) => return Err((error, surface)), | |
| }; | |
| let context: &mut HardwareBufferContext = context.try_into().map_err(|error| (error, surface))?; |
(it might also need as_mut)
There was a problem hiding this comment.
I did try this initially, but it doesn't work, because surface moves into the closure and the method itself is taking ownership and needs to use it later in the non-error path.
Signed-off-by: Martin Robinson <martin@abandonedwig.info>
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Thanks for the review. I think I've addressed the comments that I could.
| descriptor: &ContextDescriptor, | ||
| share_with: Option<&Context>, | ||
| ) -> Result<Context, Error> { | ||
| let share_with = match share_with { |
There was a problem hiding this comment.
Oh, cool! I've done this everywhere.
| let context: &mut AngleContext = context.try_into()?; | ||
| if context.egl_context == egl::NO_CONTEXT { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
This was to avoid unpacking the AngleContext more than once (unbind_surface_from_context and destroy_surface both take a Context). I've moved this back to the origin order and now just unpack it more than once, the first time non-mutably.
| let context: &mut HardwareBufferContext = match context.try_into() { | ||
| Ok(context) => context, | ||
| Err(error) => return Err((error, surface)), | ||
| }; |
There was a problem hiding this comment.
I did try this initially, but it doesn't work, because surface moves into the closure and the method itself is taking ownership and needs to use it later in the non-error path.
2 participants
This requires a bit more error handling than before, but some of this
will be cleaned up once all data structures are converted to enums.