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

macro_rules!-based bits! macro #34

Closed
wants to merge 9 commits into from
Closed

Conversation

mystor
Copy link
Contributor

@mystor mystor commented Dec 14, 2019

This adds a new macro_rules! based parser for parsing bits!.

Marking as WIP, as some things are still slightly broken or unsupported.

@codecov
Copy link

codecov bot commented Dec 14, 2019

Codecov Report

Merging #34 into master will increase coverage by 1.1%.
The diff coverage is 98.76%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #34     +/-   ##
=========================================
+ Coverage   58.42%   59.52%   +1.1%     
=========================================
  Files          32       34      +2     
  Lines        3699     3770     +71     
=========================================
+ Hits         2161     2244     +83     
+ Misses       1538     1526     -12
Impacted Files Coverage Δ
src/lib.rs 100% <ø> (ø) ⬆️
src/store.rs 70.37% <ø> (ø) ⬆️
src/pointer.rs 77.35% <ø> (ø) ⬆️
src/fields/permutation_tests.rs 100% <ø> (ø) ⬆️
src/slice/tests.rs 100% <100%> (ø) ⬆️
src/vec.rs 59.09% <100%> (+19.74%) ⬆️
src/domain.rs 85.1% <100%> (ø) ⬆️
src/indices.rs 69.56% <100%> (+1.08%) ⬆️
src/macros.rs 100% <100%> (ø) ⬆️
src/macros/internal.rs 100% <100%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00bfa3c...e201f1f. Read the comment docs.

@myrrlyn myrrlyn self-assigned this Dec 14, 2019
src/macros.rs Outdated
Comment on lines 149 to 154
(Local, $bits:path, $len:expr, $slice:expr) => {
&$crate::slice::BitSlice::<
$crate::order::Local,
$bits,
>::from_slice($slice)[.. $len]
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variant can be removed, and all variants can be changed to take bits as a :path

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this whole macro can be removed, since it's no longer delegating to any other macros and just always calling the from_slice function…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason it exists is to ensure that the correct prefix is added to Local, Lsb0, and Msb0 orderings.
Without it it would be possible to write Lsb0, but have a different order with that name currently in scope, and the macro would do the wrong thing (as it would assume we wanted the internal Lsb0 ordering, but would build a BitSlice with the wrong type parameter).

src/macros.rs Outdated
// bitvec![ endian , type ; 0 , 1 , … ]
( $order:path , $bits:ty ; $( $val:expr ),* ) => {
bitvec![ __bv_impl__ $order , $bits ; $( $val ),* ]
($order:ident, $bits:ident; $($val:expr),* $(,)?) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably have cases for Local, like bits! does.

Alternatively, all of the bitvec![1, 2, 3] variants could call bits! under the hood, as they don't need a dynamic length (which is the only thing bits! doesn't support).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, once I think bits! is stabilized enough I want bitvec! to forward to it and just run Self::from_bitslice on the output. Reduces argparse code and forces me to ensure that the library implementation is actually good.

src/macros.rs Outdated
Comment on lines 460 to 479
// Ensure literal `Msb0`, `Lsb0`, and `Local` always map to our internal
// definitions of the type, as we've been structurally matching them.
(Lsb0, $bits:ident, $slice:expr) => {
$crate::vec::BitVec::<
$crate::order::Lsb0,
$bits,
>::from_slice(&$slice[..])
};
(Msb0, $bits:ident, $slice:expr) => {
$crate::vec::BitVec::<
$crate::order::Msb0,
$bits,
>::from_slice(&$slice[..])
};
(Local, $bits:ident, $slice:expr) => {
$crate::vec::BitVec::<
$crate::order::Local,
$bits,
>::from_slice(&$slice[..])
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd also be fine to make bits take a Path here. We only need it to be an ident for the interface, because only Local, u8, u16, u32, and u64 are supported.

The `__bits_from_slice!` macro ensures that the literal tokens used to
accelerate known orderings are correctly mapped to `bitvec` types, and
not to any other names that may be in the caller's scope.

Because the fundamental types' `from_*e_bytes` constructors are not
`const fn`, their presence in literal token sequences did not permit the
compiler to elevate the literal to the `'static` lifetime. This meant
that using `bits!` to return a `&BitSlice` reference over the arrays
generated by the macro failed, as the array literals went out of scope
after the `&BitSlice` constructor call exited rather than being hoisted
for the duration of the reference's existence.

In order to circumvent this, the `from_*e_bytes` constructors are
explicitly written out as `const fn` items, and a new helper macro is
written in order to dispatch token chunks to the correct constructor.
This enables the `bits!` macros to create `static DATA` bindings for the
constructed array literals, and thus make the macro return
`&'static BitSlice`.

In the future, the syntax may be extended to permit mutable buffers.
@myrrlyn myrrlyn marked this pull request as ready for review December 20, 2019 17:47
@myrrlyn
Copy link
Collaborator

myrrlyn commented Jan 7, 2020

Alright, I'm happy with this. I'm going to pull it into trunk, and call any further tinkering I want to do "maintenance" or "refinement".

Thanks so much for making the macro; I never would've thought of how to go about doing that work, and doing it in macro_rules! instead of a proc-macro is way better.

@myrrlyn myrrlyn closed this Jan 7, 2020
@myrrlyn
Copy link
Collaborator

myrrlyn commented Jan 7, 2020

I'm rebasing all of recent trunk, including this branch, atop a hotfix commit (to bump the radium dependency), so, throw away your branches.

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

2 participants