Remove num-derive crate dependency

[Jasper-Bekkers]

This PR removes the num-derive dependency and instead opts to auto-generate the implementation for num_traits::FromPrimitive manually during the autogen stage. This should save downstream dependencies from compiling a copy of syn that's dragged in through num-derive.

For some reason this also causes a whitespace diff in rspirv/dr/autogen_operand.rs. I've left it in the commit for now.

[kvark]

Wow, managing 2.5K more LOC is not something taken lightly. Is there a way to reduce this?

[kvark]

Ok, I think I know of a better way.
Looking at the code now, it does a bunch of if/else blocks. A minor improvement would be to turn them into a match. A major improvement would be to just turn this into a transmute. Something like this:

fn from_primitive(num: u32) -> Self {
  if num < $num_variants {
    unsafe { mem::transmute(num) }
  } else {
    panic!("Unexpected value")
  }
}

[Jasper-Bekkers]

Depends on where you want to go with this tbh, this generates identical code to what the FromPrimitive derive used to do.

Personally I'd be totally find just doing a transmute, but some of the enums have gaps in them (see ExecutionMode::LocalSizeHintId and the next one ExecutionMode::PostDepthCoverage) so doing a simple range check might not work properly.

[kvark]

Would it be possible to detect if there are gaps and fall back to a match for those cases?

[Jasper-Bekkers]

I've switched them to match statements which may already be a bit nicer on the eyes. I'm not sure if doing a gap-detect would be worth it since that'd add extra code to the code generator (e.g. the code you'll actually maintain) instead of the generated code.

[kvark]

@Jasper-Bekkers wouldn't it actually make the run-time faster if we do a transmute for most cases, comparing to a match? This is much more important than a few lines of code to maintain in the autogen.

[Jasper-Bekkers]

The rust compiler already takes care of doing range checks: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=f569354c173b1e1aeb06ebd11f7ecd9d

[kvark]

@Jasper-Bekkers thank you for addressing my concerns!
I still wonder if we could go with transmute in most cases. Isn't the logic of figuring out the gaps is simple if last_value + 1 != number_of_values? If it's as simple, we should totally go for it and save 1K+ LOC generated lines

[Jasper-Bekkers]

From a quick glance, it looks like only a handful of small enums actually have that property. The gaps exist because of the vulkan extension model so it's likely that all/most of these end up with gaps in them in the long run. Besides you'd add a bunch of unsafe code for cases where the compiler already does something good & clever.

[kvark]

Ideally what I'd want to have is:

match value {
  0 .. LAST_CORE_VALUE => Some(unsafe { mem::transmute(value) }),
  EXT_VALUE_1 => Some(Value::Ext1),
  EXT_VALUE_2 => Some(Value::Ext2),
  _ => None,
}

This would be concise, future-proof, and easy to read.
But I understand that this is a little more work than you probably hoped, so I don't want to block on that.
@antiagainst @aioob do you guys feel strongly either way?

[antiagainst]

I don't have a strong preference here given this is generated code. So I'm fine to land this and then revise later. :)

[Jasper-Bekkers]

Is there anything else that needs to be done before merging this in?

[Skepfyr]

This looks fine to me, the only other thing is could you remove the num dependency from spirv_headers now? It isn't used anywhere. You could also change the num dependency in rspirv to num-traits. These two together shave over a quarter off my clean compile times, so it seems worth it.

[Jasper-Bekkers]

Turns out num is still needed for the rspirv project, but you're right about spirv_headers.

[Skepfyr]

You can replace the num dependency with num-traits and change one line in the autogen, but don't worry too much I'll do it as a separate PR if you leave this one as is.

[Jasper-Bekkers]

Cool then feel free to merge this I guess :-)