Derive convenience traits for PSO components and a few other types

[ebkalderon]

Changed

• Derived Clone, Copy, Debug, Eq, Hash, and/or PartialEq for a bunch of structs, mostly within the gfx::pso::* module.
• Updated the gfx_pipeline! macro to automatically derive Clone, Debug, and PartialEq for the resulting Data, Meta, and Init structs.
• Updated the gfx_impl_struct_meta! macro to automatically derive Clone, Copy, Debug, and PartialEq for the resulting struct.
• Minor reformatting.

Fixed

• Eliminated TODOs for manually implementing Eq, Hash, and PartialEq on several structs which contain PhantomData<T> as data members.

CC @kvark @msiglreith

[kvark]

There is a lot of structs with PhantomData that you added derives to. This is not perfect. Instead, we can have a macro for the simple case of something+phantom structs, macro generating the implementations of Eq, Hash, etc.

…

On May 2, 2017, at 08:27, msiglreith ***@***.***> wrote: @msiglreith approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ebkalderon]

@kvark Good idea! I can add such a macro, if you want, but in what crate/module would you like me to store it? Using a macro certainly sounds appealing in the long-term, but the effort of wiring it up and exporting it between crates might outweigh the advantages. I don't mind fixing each of the #[derive] statements by hand. grep and vim are my best friends right now. 😂

EDIT: Looking at the source of the std::cmp::Eq trait, it appears that deriving it doesn't make a difference at all from implementing it manually since it has no methods and depends on PartialEq. As such, I think it's okay to keep #[derive(..., Eq, ...)].

[kvark]

I can add such a macro, if you want, but in what crate/module would you like me to store it?

gfx_core

but the effort of wiring it up and exporting it between crates might outweigh the advantages

It's just a few lines of code, and the only crate that would need to use it from the core is gfx itself. I don't see the effort concern here, it would save a lot of lines and make it more consistent.

As such, I think it's okay to keep #[derive(..., Eq, ...)]

Yep.

[ebkalderon]

Heh, interesting timing! Just as you commented this message, I pushed my manual fixes to my branch.

Fair enough, I'll revise it once more and add the macro then. Would lib.rs be good place or would you prefer I create a new macros.rs file instead?

[kvark]

I think the macros is small enough so that it can live in lib.rs itself.

[torkleyy]

There is a lot of structs with PhantomData that you added derives to. This is not perfect.

I don't understand, what is the problem of deriving for a struct with PhantomData?

[kvark]

@torkleyy it's not perfect in a way that these structs look very similar, and we end up with tons of code duplication. Nothing is wrong about PhantomData in particular.

[msiglreith]

As followup to @torkleyy 's question:
What was the original reason for these TODOs
// TODO: manual Eq & Hash impl because PhantomData

The Eq implementation of PhantomData returns always true and the hashing is a no-op.
Therefore #[derive(..., Eq, Hash, ...) looks fine to me instead of manually implementing them. I assumed there was some issue with these based on the TODO above.

[ebkalderon]

@msiglreith I'm pretty sure that the original commenter was mistaken about Eq in particular; I mentioned that already a few posts up and revised my PR to just derive Eq like normal.

@kvark Just curious, but if the structs in question contained a PhantomData field, doesn't that mean that it's meant to be type checked at compile time? If so, why are we skirting around type checking the phantom fields rather than deriving PartialEq and Hash? Is it for ensuring that Texture<R, T> and RawTexture<R> result in the same equality and hash?

[kvark]

Therefore #[derive(..., Eq, Hash, ...) looks fine to me instead of manually implementing them. I assumed there was some issue with these based on the TODO above.

It would be safest to look at the code that #derive produces, to make sure no extra bounds are put.
We could also have simple unit tests that just try to use the derived traits with T type that doesn't implement anything.

[kvark]

@ebkalderon

If so, why are we skirting around type checking the phantom fields rather than deriving PartialEq and Hash?

We aren't cheating here. The limitation of #derive, AFAIK, is that it automatically requires the generic type parameters to implement the traits you specify, while the actual implementation may not even care. So we should be smarter than #derive by macroing it manually.

[torkleyy]

@kvark Indeed (https://is.gd/7fse6V), but why is that? It could just do a where PhantomData<T>: Hash, right?

[kvark]

@torkleyy surely it could be better... the answer lies somewhere within the #derive implementation. And I have a feeling that fixing that would break rust-1.0 promise.

[torkleyy]

Yeah, it does:

use std::marker::PhantomData;

struct Foo;

#[derive(Clone, Copy, Hash, PartialEq, Eq)]
struct Bar<T> {
    value: u32,
    phantom: PhantomData<T>,
}

impl Hash for Bar<Foo> { // Conflicting impls if #derive did the right thing
    ...
}

[ebkalderon]

Relevant issue: rust-lang/rust#26925

[kvark]

Happy to see this coming to a success!
@homu r+

[homu]

📌 Commit b31869a has been approved by kvark

[homu]

⌛ Testing commit b31869a with merge 162221c...

[homu]

☀️ Test successful - status