[abrown]

• This has been discussed in issue Remove dependency on packed_struct? #1271.
• A short description of what this does, why it is needed:
  This re-factors EncodingBits to remove the dependency on packed_struct; the result is more lines of code, which is unfortunate.
• This PR contains test cases, if meaningful.
• A reviewer from the core maintainer team has been assigned for this PR.

[abrown]

@sstangl, @bnjbvr: I pushed this so you could observe this failure:

thread 'main' panicked at 'trying to insert different recipes with a same name (Op1ldWithIndex)', cranelift-codegen/meta/src/isa/x86/encodings.rs:46:13

I've looked over the code I changed and I don't exactly understand why the following assertion should fail due to my changes:

cranelift/cranelift-codegen/meta/src/isa/x86/encodings.rs

Lines 46 to 52 in 6e2bffc

	assert!(
	self.recipes[*found_index] == recipe,
	format!(
	"trying to insert different recipes with a same name ({})",
	recipe.name
	)
	);

[edit: I thought the assert was wrong but on second thought it isn't; I now have less of an idea why the build fails]

[abrown]

I don't exactly understand why the following assertion

Never mind, I pushed a new version with a corrected constructor. Hopefully you all can see any other potential issues in the bit-fiddling logic (write and read).

[sstangl]

Looks good to me! Thanks for removing that.

Just some minor changes to the masking, and undefined behavior could be caused by some RangeInclusive inputs.

[sstangl]

The RangeInclusive<u16> doesn't actually enforce that start <= end. It merely guarantees that if given a range with an empty inclusive range, as an iterator it will not yield any elements.

However, if given such a range, the documentation for RangeInclusive::start() and RangeInclusive::end() say that calling start() and end() yield unspecified values. The documentation says that before using those functions, the existence of a valid range must be checked explicitly via is_empty().

I do understand that the ranges are controlled here, but I'd still recommend checking the invariants as the docs suggest.

[abrown]

Maybe it's a Rust version issue or something (I'm on 1.39.0) but when I add the assert I see the following error:

error[E0034]: multiple applicable items in scope
  --> cranelift-codegen/shared/src/isa/x86/encoding_bits.rs:70:24
   |
70 |         assert!(!range.is_empty());
   |                        ^^^^^^^^ multiple `is_empty` found
   |
   = note: candidate #1 is defined in an impl for the type `std::ops::RangeInclusive<_>`
   = note: candidate #2 is defined in an impl of the trait `std::iter::ExactSizeIterator` for the type `std::ops::RangeInclusive<u16>`
   = help: to disambiguate the method call, write `std::iter::ExactSizeIterator::is_empty(range)` instead

When I take the compiler's advice I still see the error and adding feature flags as in rust-lang/rust#48111 (comment) does not seem to work either. I may have to create a new issue for this...

[abrown]

Opened #1284.

[philipc]

Both is_empty methods are still unstable. Maybe use range.len() instead of calculating the size yourself?

[abrown]

Does it have len? I can't seem to find it...

[philipc]

I haven't tried, but it should have it via the ExactSizeIterator impl.

[abrown]

Thanks! I think that works.

[bnjbvr]

Removing myself from the reviewers, since Sean has taken care of it, thanks! Just a side comment: can you remove the dependencies section in the meta-shared crate Cargo.toml, and add a comment in there that we should really strive to not having any dependencies in this crate? (Since it blocks both the meta and non-meta codegen crates to compile, and these are slow to build)

[bnjbvr]

Thanks for doing this, @abrown !