Implement Clone for Module

[daxpedda]

Allows Module to be cloned.

[teoxoy]

I think we should implement this via a feature flag that's on by default (and run CI with default features off) since @kvark previously mentioned that having these Clone implementations makes it too easy for the contributors of the project to clone potentially expensive structures unnecessarily.

[daxpedda]

I used cfg(dev) instead, I hope that's appropriate.

[teoxoy]

Oh, I think that's even better!

[teoxoy]

LGTM but let's wait for one more review

[JCapucho]

I think we should do the opposite and add a feature flag to enable it that's not the default, because I think most people that would want to dabble in naga will try to run

$ cargo run

And this will cause them to be able to Clone even though they shouldn't and then later when they try to open a PR it will fail on CI and debugging it would be a little bit counter intuitive.

But either way we should document this feature flag

[daxpedda]

I don't particularly understand the problem, calling clone() is explicit, why are we afraid of people misusing it or not be able to catch it in code review? I mean Vecs and HashMaps are also cloneable and they can be pretty large too, any clone() call should be paid attention to.

I think we should do the opposite and add a feature flag to enable it that's not the default, ...

This PR is specifically made for gfx-rs/wgpu#2903, so not having it on by default doesn't really work.

Alternatively, instead of Clone, I could also implement ToOwned, this should stand out even more.

Otherwise I could change gfx-rs/wgpu#2903 to not use Cow, but some custom type not requiring ToOwned or Clone.

But either way we should document this feature flag

I could add a note under README#development-workflow?

[JCapucho]

I don't particularly understand the problem, calling clone() is explicit, why are we afraid of people misusing it or not be able to catch it in code review? I mean Vecs and HashMaps are also cloneable and they can be pretty large too, any clone() call should be paid attention to.

While Clones are explicit they are a tool that people can easily reach to and will do so, and naga doesn't clone IR internally for memory and performance considerations.

And while we can review them, doing so in every PR and pointing it out every time is an added task to the reviewer and some are voluntary so easing their work is always better and it's also unexpected to the contributor because they saw nowhere that the codebase doesn't accept it and suddenly are being told to not do it.

This PR is specifically made for gfx-rs/wgpu#2903, so not having it on by default doesn't really work.

Alternatively, instead of Clone, I could also implement ToOwned, this should stand out even more.

Otherwise I could change gfx-rs/wgpu#2903 to not use Cow, but some custom type not requiring ToOwned or Clone.

I don't understand the problem. wgpu is a user of naga so it can add to it's Cargo.toml the feature explicitly and I assume it already does so for other features.

No need to implement other traits.

But either way we should document this feature flag

I could add a note under README#development-workflow?

If we do end up following the negative feature flag then yes this would be the preferred place

[daxpedda]

I don't understand the problem. wgpu is a user of naga so it can add to it's Cargo.toml the feature explicitly and I assume it already does so for other features.

Yeah, I don't know what I was thinking, will do that.

[daxpedda]

Alright, I changed it to a non-default crate feature and documented it in the crate documentation!

[JCapucho]

I'll just wait for @teoxoy to re-review since he originally approved it but otherwise it seems good to me, thanks for your contribution.

[teoxoy]

Looks good; I think the previous cfg(dev) could have clashed with downstream users possibly setting the cmd line flag --cfg dev themselves.