Traverse-Research / rspirv-reflect

🦀 Minimal SPIR-V reflection library.
https://traverseresearch.nl
Apache License 2.0
41 stars 10 forks source link

Add Send & Sync for ReflectError to support ? with anyhow::Result #24

Closed BeastLe9enD closed 2 years ago

BeastLe9enD commented 2 years ago

Hey, In the following code does not compile at the moment:

use std::fs::File;
use std::io::Read;
use anyhow::Result;
use rspirv_reflect::Reflection;

fn main() -> Result<()> {
    let mut file = File::open("test.vert.spv")?;

    let mut buffer = Vec::new();
    file.read_to_end(&mut buffer)?;

    let reflection = Reflection::new_from_spirv(&buffer)?;
    let descriptor_sets = reflection.get_descriptor_sets()?;

    println!("{:#?}", descriptor_sets);

    Ok(())
}

It fails with error[E0277]: `(dyn std::error::Error + Send + 'static)` cannot be shared between threads safely

Therefore, I added Send & Sync for ReflectError to allow support for anyhow::Result. From what I saw, there shouldn't be any error sending ReflectError to multiple threads, correct me please If I'm wrong with this assumption!

BeastLe9enD commented 2 years ago

Hey, just wanted to ask if I should close the pull request or if it just hasn't been processed yet :)

MarijnS95 commented 2 years ago

This PR contains way more changes than originally described in the title/description. Please reset this branch (you seem to have created the PR directly from main :fearful:) back to e48dd3af384af925b651079f8f319550b2b1775b and open separate PR's for every logical feature you'd like to get in (all with individual title/description/justification), thanks!

Then, the compiler automatically implements Send/Sync when all members implement it. Lack thereof means one or more (variant) members are not actually Send/Sync and require you to put in the work and carefully validate plus clearly describe why you think it can be unsafely implemented. You mentioned you "saw there shouldn't be any error", please document your findings explicitly. Good luck :crossed_fingers:!

BeastLe9enD commented 2 years ago

Yes, I added a few little things, but of course I'm happy to do them individually if that's what you want. I'll clean up a bit tomorrow, then you'll hear from me :)

BeastLe9enD commented 2 years ago

Im closing this for now, as https://github.com/gfx-rs/rspirv/pull/234 solves this issue!