Make strlen more like musl version

[bombless]

I think there are 2 tricks here:

• If you read from memory, it is best that the memory address is aligned to word-boundary
• If you check a string of memory and you can check N bytes within the same time, you are N times faster than you check one byte at a time

[bombless]

I didn't add tests directly but I do did some unit tests: https://play.rust-lang.org/?gist=1953956b2b941525bb89abcda6925db1&version=nightly&backtrace=0&run=1

[anp]

Thanks for the PR! I need to document this, but I'm relying on the test suite used for musl to check any changes. If you're on Linux (or a Linux VM), you should be able to run ./build_and_test.sh from the project root directly, which will build musl and rusl, and run libc-test on the combined archived. Could you give that a try? I'll give it a go sometime soon when I have some time to clone this down.

[anp]

I think these three items (align, ones, highs) would be more appropriate as const's instead of macros.

[bombless]

I'm afraid CTFE of Rust is not powerful enough.
See rust-lang/rfcs#322

[anp]

Ah yes, size_of won't be const for a while. That said, I still don't think that macros add anything here, and make it less readable. Perhaps these could be instantiated as stack variables instead?

[anp]

IMO, inferred casts are not particularly well aligned with one of the things I've enjoyed about writing in Rust -- it encourages explicit casts to make it clear what conversions are involved.

[bombless]

Nits addressed

[bombless]

http://doc.rust-lang.org/reference.html#conditional-compilation
I think we can replace these inlined functions to const if we make use of target_pointer_width

[bombless]

Ah I forgot to add tests.
I'll try to run the test once I find a Linux machine, within a few days.
Or feel free to take over this PR.

[bombless]

https://crates.io/crates/pointer-width
made a crate to provide pointer size at compile-time
not sure if it's good idea though

[eddyb]

@bombless Isn't this what std::usize::BYTES was?

[bombless]

Thanks @eddyb, but isn't this feature deprecated?
rust-lang/rust#27753

[anp]

Looks great, thanks! I'll run this through the test suite soon.

[anp]

Because this isn't a root module, the core namespace isn't automatically available, and this won't build. Changing it to this works fine:

#[inline]
fn align() -> usize {
    use core::mem;
    mem::size_of::<size_t>()
}

[bombless]

Done that.
But I'm not sure how it fix the problem.

I see core::usize is at the top of this strlen.rs originally, so I guess ::core is already available?

I should check the project structure at first though, I'll spend some time to investigate.

[bombless]

src/api/fcntl.c:30:1: note: each undeclared identifier is reported only once for each function it appears in
src/common/runtest.c:29: ./src/api/main.exe exec failed: No such file or directory
FAIL ./src/api/main.exe [status 1]
gcc: error: src/api/fcntl.o: No such file or directory

Looks wierd?

[anp]

Unfortunately, musl doesn't pass all of it's own tests (here's a list of ones which fail on x86_64 with the pure C version: https://github.com/dikaiosune/rusl/blob/master/baseline_failures). The tests actually "pass" in that they don't regress any further.

The CI build failed because I just now added a script which gates the build on rustfmt having nothing to change. Could you run the patch through rustfmt 0.5.0 (cargo fmt from the root directory is easiest as it will include the configuration file) and see if CI passes then?

EDIT: Also, sorry for not giving you a heads up on the change, I just did it earlier tonight and was in the process of closing other issues.

[anp]

Also, another heads up -- I'm going to be including clippy as a build dependency, so you may wish to hold off on further attempts until I've got that sorted out and you can rebase with the changes.

[bombless]

I'll find a Linux machine tomorrow, guess that will make things more smoothly :-)

[anp]

OK, sounds good. I'll probably have clippy set up by then, so I imagine you'll want to cargo install clippy as well.

[eddyb]

Isn't ones() & x == x? Seems unnecessary here.

[ConnyOnny]

has_zero is easily answered: a binary numer has a zero if it's not completely ones, so x != ones() is all it takes for this function.

[ConnyOnny]

Ah, but you're looking for zero bytes. Maybe call the function has_zero_byte? ;-)

[eddyb]

I went back and looked at the C version:

#define ALIGN (sizeof(size_t))
#define ONES ((size_t)-1/UCHAR_MAX)
#define HIGHS (ONES * (UCHAR_MAX/2+1))
#define HASZERO(x) ((x)-ONES & ~(x) & HIGHS)

That's quite different from what's in this PR. In Rust, it would be:

let align = mem::size_of::<usize>();
let ones = usize::MAX / (u8::MAX as usize);
let highs = ones.wrapping_mul((u8::MAX / 2 + 1) as usize);
let has_zero = |x: usize| (x.wrapping_sub(ones) & !x & highs) != 0;

However, this can be made much simpler:

const ONES: usize  = 0x0101010101010101_u64 as usize;
const HIGHS: usize = 0x8080808080808080_u64 as usize;
let has_zero = |x: usize| (x.wrapping_sub(ONES) & !x & HIGHS) != 0;

[eddyb]

You can also use const HIGHS: usize = ONES << 7; if you want to avoid having more than one constant at a time. I also don't think align needs to be out of line, calling size_of directly should be fine.