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

Support bitflags #528

Merged
merged 6 commits into from
Jul 15, 2021
Merged

Support bitflags #528

merged 6 commits into from
Jul 15, 2021

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Jul 10, 2021

Closes #307

TODO:

  • check that #[cfg] on bitflags consts works

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

I have done a first pass on this PR and will continue tomorrow (probably) but I have commented on things that looked a bit odd to me.

decoder/src/lib.rs Outdated Show resolved Hide resolved
macros/src/bitflags.rs Show resolved Hide resolved
macros/src/bitflags.rs Outdated Show resolved Hide resolved
macros/src/bitflags.rs Show resolved Hide resolved
macros/src/bitflags.rs Show resolved Hide resolved
@jonas-schievink jonas-schievink changed the title Bitflags ¯\_(ツ)_/¯ Support bitflags Jul 14, 2021
@jonas-schievink jonas-schievink marked this pull request as ready for review July 14, 2021 13:26
Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! I gave this a second pass and it looks good to me. Left some inline comment about the code. I'd block landing this on adding documentation (API + book); feel free to send this PR to bors when docs are in place.

Feel free to ping me if you'd like some more review on future changes.

decoder/src/elf2table/mod.rs Outdated Show resolved Hide resolved
decoder/src/elf2table/mod.rs Outdated Show resolved Hide resolved
macros/src/symbol.rs Show resolved Hide resolved
parser/src/lib.rs Show resolved Hide resolved
decoder/src/lib.rs Show resolved Hide resolved
}
}

#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

could you move the parsing implementation into its own module and e.g. have Input / Flag expose public accessors for the parts that codegen needs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to a submodule, but kept it in the same file

src/lib.rs Outdated Show resolved Hide resolved
match self.table.bitflags.get(&key) {
Some(flags) => {
// FIXME flag print order does not match definition order
// (does some part of the toolchain alphabetically sort them?)
Copy link
Member

Choose a reason for hiding this comment

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

interesting observation: perhaps the linker sorts the symbols alphabetically within a section? maybe we could prefix the symbols (00_$first, 01_$second, etc.) so that even after sorting alphabetically they stay in the declaration order? that doesn't have to be addressed in this PR but we should create an issue for this

decoder/src/frame.rs Show resolved Hide resolved
Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

still looks good to me

@japaric
Copy link
Member

japaric commented Jul 15, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

Build succeeded:

@bors bors bot merged commit 5089314 into knurling-rs:main Jul 15, 2021
@jonas-schievink jonas-schievink deleted the bitflags branch July 15, 2021 17:30
bors bot added a commit that referenced this pull request Sep 8, 2021
570: Support referring to `Self` in bitflags constants r=Urhengulas a=jonas-schievink

The initial bitflags support in #528 had a restriction where referring to the `Self` type in a `bitflags` constant would result in a compile error.

Turns out that's entirely unnecessary and I wasn't quite awake when I wrote that code. A minor modification of the generated code avoids this somewhat annoying restriction. Instead of copying the expression verbatim, we can just refer to the associated constant created by the `bitflags` crate, and extract the raw bits.

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
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.

bitflags! support
2 participants