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

Constants deserialization #147

Closed
kvark opened this issue Jan 7, 2018 · 12 comments
Closed

Constants deserialization #147

kvark opened this issue Jan 7, 2018 · 12 comments

Comments

@kvark
Copy link

kvark commented Jan 7, 2018

Currently, serializing any bit flags into a text format isn't exactly readable:

		"im-color": Image(
			kind: D2(1, 1, Single),
			num_levels: 1,
			format: Rgba8Unorm,
			usage: (bits: 4), // <----
		),

It would be super useful to support given constants for deserialization (and potentially serialization too?), making this:

			usage: COLOR_ATTACHMENT,

I don't know if it's best to approach this problem within the implementation of derive(Deserialize) macro in the rust repo, user-side things like derivative, implementation of particular format in use (RON), or by changes in this care. Filing the issue here because bitflags are the strongest example for the case.

@torkleyy
Copy link

This would be really nice, but I think it becomes tricky once you're dealing with combined flags (COLOR_ATTACHMENT | DEPTH_ATTACHMENT). Otherwise I'd already be creating a PR :)

@kvark
Copy link
Author

kvark commented Jan 19, 2018

First off, the serialization is straightforward - just follow the same logic as Debug implementation does.

I think it becomes tricky once you're dealing with combined flags

For deserialization, it certainly feels possible. I could see two approaches:

  1. BitFlags level: parse an expression of type "X | Y |Z", where X, Y, Z are among the given bitflag constants.
  2. Serde level:
    • support parsing XXXX for the value, where XXXX is an associated constant of type Self for the expected deserialization type
    • support unary/binary expressions with operators: "-5", "-ONE", "4+5", "PI * 0.5", "AA | BBB"

Both approaches seem to be pretty consistent, and I don't find them tricky. Just a matter of how generic we want this to be.

@torkleyy
Copy link

torkleyy commented Jan 19, 2018

I didn't see the Debug implementation. I wouldn't call it straightforward though.

The most reasonable implementation is using a string IMO. Would be nice to hear some other opinions.

@kvark
Copy link
Author

kvark commented Jan 19, 2018

@torkleyy

I wouldn't call it straightforward though.

It doesn't matter, really, if this one is not straightforward. My point is - the code is already there, we just need to reuse it. In the worst case, the serializer can always bail out and write the bits directly (like it does today), with an ability to improve later. So I don't see this being a problem.

@kvark
Copy link
Author

kvark commented Jan 28, 2018

I've implemented the requested logic in a wrapper macro: https://github.com/kvark/bitflags-serial
Any feedback is appreciated!

If you'd like to move the code into bitflags (behind serde as a feature), I'd be happy to make a PR. The benefit of this is that I only need to intercept a single macro __impl_bitflags, where if I do a wrapper I need to support all the syntax variants of bitflags.
If not, let's close this.

@kvark
Copy link
Author

kvark commented Feb 16, 2018

@alexcrichton @dtolnay need your input here ^

@LegNeato
Copy link

LegNeato commented Mar 2, 2018

This is better / more generic, right? I would almost call the current behavior a bug. If my rust code changes values but keeps the label constant, deserializing from storage will set the correct flags. With the current serialization the wrong flags will be set.

@kvark
Copy link
Author

kvark commented Mar 8, 2018

@LegNeato I don't find this to be more generic. The main gain is convenience (for an ability to use constants by names instead of values). I do agree that the current behavior can be error prone though.

bors bot added a commit to gfx-rs/gfx that referenced this issue Apr 19, 2018
1954: [dx12] layer clears r=msiglreith a=kvark

Fixes #1943, works around #1945
PR checklist:
- [ ] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends: dx12

I found out that we didn't update the raw bit flag values in reftests after one of the recent changes... Unfortunate, but this is more of a bitflags issue than ours, and I'm pushing upstream to resolve this in bitflags/bitflags#147


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@kvark
Copy link
Author

kvark commented Feb 4, 2021

Can we get some movement on this? Maintaining a fork of bitflags doesn't sound appealing to me.
So, again, to re-iterate the points: Debug implementation already has all the logic for serializing, there is nothing new here.
Deserialization is trickier, but perhaps we could have something that works for a few popular cases, and is exposed behind a feature? I think there is a huge value in it.

@KodrAus
Copy link
Member

KodrAus commented Oct 24, 2021

If it's possible for us to support this without breaking existing serialized values (I think non-self-describing formats like bincode can be tricky because we can't switch on the type of input we receive) then it seems like a good idea to me.

Note though that our Debug implementation is slightly broken in #137. Now that #220 is merged though, it could be possible to come up with a generic implementation over that that you could use with #[serde(with)] to get much nicer serialized values.

@KodrAus
Copy link
Member

KodrAus commented May 4, 2022

One of the goals of our 2.0.0 release is to separate the public flags from a private type that we control through the macro. This means we would be able to add a FromStr implementation to that private type without users having to expose it if they don't want. Along with #278 that lets you iterate over set flags and their names we should be able to neatly support serialization using formatted strings.

@KodrAus
Copy link
Member

KodrAus commented Mar 13, 2023

This is supported in 2.0.0 now, where human-readable formats will serialize flags as case-sensitive, bar-separated strings with additional bits as hex-coded numbers like:

A | B | 0x0c

@KodrAus KodrAus closed this as completed Mar 13, 2023
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

No branches or pull requests

4 participants