hecatia-elegua / bilge

Use bitsized types as if they were a feature of rust.
Apache License 2.0
171 stars 17 forks source link

Add "fallback" attribute for an enum variant, allowing derive(FromBits) even if unfilled #23

Closed pickx closed 1 year ago

pickx commented 1 year ago

Resolves #24. Currently, enums which don't fill their bitfield don't allow deriving FromBits. This PR allows adding a #[fallback] attribute to one (and only one) variant of such enums:

#[bitsize(3)]
#[derive(FromBits)]
enum Opcode {
    Add = 0b001,
    Sub = 0b010,
    Mul = 0b011,
    #[fallback]
    Reserved,
}

This will then generate a _ => Self::Reserved arm in the From<int> implementation. This is is a followup from the reddit thread, where I realized this feature could be useful to me.

Please note that this is a WIP: for example, this trips the generated assert!(#type_name::FILLED) check, and so the generated code fails to compile. I wanted to see if this PR aligns with what you want this crate to look like, before attempting to change that assert.

I'm open to any changes or suggestions. Thanks for making this crate.

pickx commented 1 year ago

Hey, thanks for the quick and thorough response. I do think it makes sense to re-introduce the separation between the two derives. As you have pointed out, #[reserved] shouldn't be allowed with TryFromBits (and it should abort to inform the user about this). Thus the try_from_bits.rs doesn't need to know about this attribute and we can keep the whole reserved variant logic inside from_bits.rs. Following this line of thought, maybe analyze_enum_derive() should be split into two versions and not take try_from. I'm not sure.

Regarding the name, reserved does seem like it could clash with some other crate's attribute. I chose that name because it already has a similar meaning in bilge's structs. fallback also sounds good to me but is just as likely to clash.

As for FILLED - generating assert!(true) would work. Another, possibly hackier option could be to also generate a const RESERVED: Option<Self> in the impl, which would be the reserved variant or None if no #[reserved]. Then the assert would be assert!(#type_name::FILLED || #type_name::RESERVED.is_some()). This has the benefit (?) of also letting the enum know about its reserved variant after generation, which I guess might be useful somehow.

hecatia-elegua commented 1 year ago

Actually, #[reserved] is from_bits enum-only, so, without looking at the code again, can we just put it in the from_bits.rs analyze_enum code path? I also agree with really going for the split. Back then I also regretted a bit that I handled it with a bool, I should've just called multiple smaller functions in shared.rs instead. If you're up to it, do it, otherwise I can do it myself after this PR.

I like fallback! Use that, it sounds super clear. I can see reserved being useful for something else.

Let's leave out RESERVED but I'll keep it in mind.

pickx commented 1 year ago

Actually, #[reserved] is from_bits enum-only, so, without looking at the code again, can we just put it in the from_bits.rs analyze_enum code path?

yep, I tried doing as much as possible, though I did made structs also check for a fallback (and so kept it in the derive analysis part), so they can fail. I hope the changes I made are not radical, obviously I'm always up for suggestions.

hecatia-elegua commented 1 year ago

Man, they really should be splitting dev-dependencies into test and bench. Or I should split benches off into another crate.

Let's merge 👍

hecatia-elegua commented 1 year ago

Thanks!

hecatia-elegua commented 1 year ago

@pickx btw did you want your name to be slapped onto these commits? Because that won't link to your github profile now, which is not so nice for you.

There's also this, which you can do with every commit on github, by adding .patch to the end of the link: https://github.com/hecatia-elegua/bilge/commit/bd2e319d22b71c814a77d26df094f6c38bafefcd.patch which leaks your e-mail (not sure if that matters to you)

pickx commented 1 year ago

@pickx btw did you want your name to be slapped onto these commits? Because that won't link to your github profile now, which is not so nice for you.

There's also this, which you can do with every commit on github, by adding .patch to the end of the link: https://github.com/hecatia-elegua/bilge/commit/bd2e319d22b71c814a77d26df094f6c38bafefcd.patch which leaks your e-mail (not sure if that matters to you)

yeah... uh, that's not what I expected to happen. I now see that my .gitconfig is to blame. sorry, but is it possible to change this to use my github account?

edit: I guess I can submit a new commit with a different e-mail? real newbie moment here. 😅

hecatia-elegua commented 1 year ago

Github is kinda dumb at stuff like this. (I have a big fat Revert button besides the merge message here, but it doesn't do the basic thing you'd think it does) I've noticed some of your commits last year have the same problem, not sure if you want to fix all those too.

You'd have to use interactive rebase on all these commits locally and change your author information, then force push. Then I'd have to do the same on the main branch. That should be it?

https://dev.to/brayanarrieta/how-to-change-the-git-commit-author-56mg vscode gitlens also has a nice UI if you do this: https://www.youtube.com/watch?v=P5p71fguFNI

pickx commented 1 year ago

@hecatia-elegua okay, I seem to have done it. for reference, this is an easy way to do it while preserving commit dates https://stackoverflow.com/a/70190223

edit: and thanks for being patient with me!

hecatia-elegua commented 1 year ago

@pickx I think it didn't work. I've added two links to the previous message, that way should work.

Oh wait you just have to force push or sth? Or does github not register that you changed it? Probably because contributors could then destroy old PR histories... hm

pickx commented 1 year ago

Oh wait you just have to force push or sth? Or does github not register that you changed it? Probably because contributors could then destroy old PR histories... hm

yeah, that seems to be it. while the branch in my fork changed, the one in this pull request didn't. my assumption is that you also have to do this on your own repo.

hecatia-elegua commented 1 year ago

Well, I've changed it on main, so it's only left in here and your old repos. If you want we can delete our comments here to make it less obvious, but I'm guessing you only wanted the github account linkage, not privacy? Although you use protonmail hmm... btw. you can message me on discord, too, if you have that (hecatia-elegua#1559) 👍

pickx commented 1 year ago

sent you a friend request (pike#8242). thanks again, that was my first open source commit (could you tell? lol). I'm fine with keeping the PR page way it is. not a big deal.