shepmaster / jetscii

A tiny library to efficiently search strings for sets of ASCII characters and byte slices for sets of bytes.
Apache License 2.0
113 stars 20 forks source link

use const generics for the `PackedCompare` control byte #39

Closed lqd closed 3 years ago

lqd commented 3 years ago

Hi Jake 👋.

I was looking at this crate in the stdarch crater results, and I'd thought I'd try to fix it since it's yours :) I'm not sure if it's still super active, or if the proposed solution would be acceptable to you (as it uses const generics and thus is technically a breaking change by bumping the MSRV). There may be other acceptable solutions of course, but this seemed clean enough to ask for your opinion.

Some generic explanations follow:


Your crate came up in the results of a crater run updating a part of libcore: stdarch, and this PR should fix that.

For context, we're trying to update libcore's stdarch to improve handling of immediate mode arguments (tracking issue: https://github.com/rust-lang/stdarch/issues/248): the current solution is suboptimal. It uses macros to ensure the immediate arguments are indeed constants, and that causes problems (the size of libcore and the time it takes to compile).

Now that const generics are available, we're trying to move from the previous way of doing things to using const generics, so that non-constant immediates cause a compile error instead:

(There is also more discussion around this topic in https://github.com/rust-lang/rust/issues/83167 if that interests you, but it is more about the best way to keep existing code working, whether a switch to const generics should be done over an edition, and so on.)

Overall, jetscii would stop compiling if that PR above were to be merged: the PackedCompare::cmpestrm{i} methods are generic and use the _mm_cmpestrm{i} intrinsics. However, they pass the control byte immediate via a generic constant and that doesn't work with min_const_generics and #[rustc_legacy_const_generics].

So, this PR adds a const generic parameter to the PackedCompare struct, and removes the CONTROL_BYTE associated constant from PackedCompareControl. It's less self-contained than before, that's for sure...

I've ran the tests in this crate using the rustc CI artifacts from that PR: thanks to rustup-toolchain-install-master, with the 9cae4dca0b4ddbfe82616f4d5f1090fc817e2ffa rustc CI revision.

Amanieu commented 3 years ago

@shepmaster Ping! Any update on this?

shepmaster commented 3 years ago

Thanks!