Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump bitflags to v2 #2358

Merged
merged 2 commits into from Jun 1, 2023
Merged

Bump bitflags to v2 #2358

merged 2 commits into from Jun 1, 2023

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented May 27, 2023

Changelog: https://github.com/bitflags/bitflags/blob/2.0.0/CHANGELOG.md#200.

This is a breaking change. The API that is implemented by the macro is now different.

I was a bit liberal with deriving std traits on public types, which mimics the behavior of bitflags v1, let me know if we don't need all these traits. Potentially we could at least drop PartialOrd and Ord.

@daxpedda daxpedda force-pushed the bitflags-v2 branch 2 times, most recently from 971ced3 to 6099be5 Compare May 27, 2023 16:50
@daxpedda
Copy link
Contributor Author

CI currently fails on master with the same error as well.

@jimblandy
Copy link
Member

Why are the serializations changing?

Why do we have all these new #[derive] attributes?

@daxpedda
Copy link
Contributor Author

daxpedda commented May 27, 2023

Why are the serializations changing?

It was changed in v2. Now human-readable formats use a string, which contain the name of the flags. JSON is a human-readable format, that's why it's affecting the tests. In bincode for example, it would still just use an integer.

See https://github.com/bitflags/bitflags/blob/2.0.0/CHANGELOG.md#changes-to-serde-serialization.

Why do we have all these new #[derive] attributes?

In v1 the bitflags! macro derived them for us. This isn't the case anymore in v2, now derives need to be added manually.

This was done in a PR like this bitflags/bitflags#282 and is unfortunately not highlighted in the changelog.

@teoxoy
Copy link
Member

teoxoy commented May 31, 2023

@daxpedda I fixed the failing CI on master, could you rebase?

@daxpedda
Copy link
Contributor Author

Done.

@daxpedda
Copy link
Contributor Author

daxpedda commented May 31, 2023

This error doesn't make a whole lot of sense to me, my PR should have no effect on this. But somehow CI on master succeeds, so what gives?

--- a/tests/out/glsl/force_point_size_vertex_shader_webgl.fs_main.Fragment.glsl
+++ b/tests/out/glsl/force_point_size_vertex_shader_webgl.fs_main.Fragment.glsl
@@ -1,4 +1,5 @@
 #version 300 es
+#extension GL_EXT_texture_shadow_lod : require
 
 precision highp float;
 precision highp int;

Should I just fix it?

@daxpedda daxpedda force-pushed the bitflags-v2 branch 2 times, most recently from b08ed0e to e5c7cc8 Compare May 31, 2023 13:38
@teoxoy
Copy link
Member

teoxoy commented May 31, 2023

I'm not sure how much value there is in deriving PartialOrd, Ord for these. @jimblandy thoughts?

Cargo.toml Outdated Show resolved Hide resolved
@ErichDonGubler
Copy link
Member

@teoxoy:

I'm not sure how much value there is in deriving PartialOrd, Ord for these. @jimblandy thoughts?

The only value I can think of for these (and Hash) is if somebody wanted to use these bitflags structures as an ordered set, or as tree keys. I don't think that that's surface we want to support right now, is it?

@jimblandy
Copy link
Member

We should derive only the traits we actually need. I believe they contribute to compile time, even if you don't use them, but more importantly, deriving them leads one to wonder what those traits are used for. Seeing that a trait is not derived is actually a helpful hint (of course, there could be impls elsewhere, so it's just a hint, but...).

@teoxoy
Copy link
Member

teoxoy commented May 31, 2023

Some of these types are public and they previously had all of those traits automatically derived by bitflags. I imagine we always want Clone, Copy, Debug, I can see people using PartialEq, Eq. PartialOrd, Ord, Hash I'm not sure if are that useful for bitfields.

@teoxoy
Copy link
Member

teoxoy commented May 31, 2023

Proposal: remove PartialOrd, Ord, Hash and if people want them back, we can add them back later

@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 1, 2023

Done.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 1, 2023

Some types require Hash as they are used in HashMaps: 89f1425.
PartialOrd and Ord could be removed by removing the requirements from types containing them.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@teoxoy teoxoy merged commit b7f4006 into gfx-rs:master Jun 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants