I would prefer to split the cosmetic changes to this function from the functional ones. E.g, have one commit that adds the __restrict and *only* adds this #ifndef, then later the all the switching around of dst and src, and all the other cosmetics.

I'm not sure what is gained here except more characters? The end cast to void * is not needed, it is implicit because of the return type. Changing the inner cast from char * to unsigned char * also does not have any effect, since the pointers are never dereferenced. The original seems clearer to me, and less obfuscated.

I take it that these are the kernel side versions, which don't attempt to load locales and that sort of stuff? Otherwise they're definitely not pure. :)

Why? Shouldn't make a difference for FreeBSD as, other than signedness, they should be the same. For CheriBSD we need to use a different type, intcap_t (which is what intptr_t is for pure-capability code so the existing code _almost_ works, except for hybrid code intptr_t remains a 32/64-bit integer, not a capability like intcap_t).

__predict_false?

This should be entirely equivalent to what was there before, and any modern compiler should be able to optimise that.

Moving these violates style(9)

Why use u_char all of a sudden? char works just fine.

Using size_t here is actually good for CHERI, we changed it downstream as otherwise you have uintptr_t | uintptr_t later, the provenance for which is ambiguous, but we don't actually need a pointer, just the low bits of it.

Why the blank line?

Why move dst and src around?

Why move?

Why the blank line?

< has become <=, I assume because you copied it from the forward case, and I think this now behaves incorrectly for an unaligned-but-by-the-same-amount (e.g. 0x1001 to 0x2001) word-sized memcpy. It works for the forward case because t = wsize - (t & wmask) gives you wsize when t == length, but in the backwards case t &= wmask gives you 0 not length (which, due to the use of TLOOP1, ends up doing SIZE_MAX+1 iterations rather than 0).

Why swap the order?

There's nothing else in the file, seems a bit unnecessary

Why? unsigned isn't needed, and implicit casts to void * are legal C.

You've lost the return NULL for if s is still NULL

__restrict needs a space before it, but touching libkern.h files without the associated changes to the implementation is wrong.

It reads better since src comes after dst

yeah it just looks cleaner

I mean it is undefined to have s and last be null according to the c standard

I mean "use the __predict_false macro rather than writing your own".

That's not a good reason to change something. Both are equivalent semantically, both get optimised to the same thing and both are easy to understand. But whether or not it's cleaner is subjective; personally I prefer what's there because it's shorter and reuses code.

Neither of these have been addressed.

Subjective. Also it was better when they were both on the same line, rather than one hiding at the end of the previous long line. Don't change code because you can.

Not true; *last is entirely unspecified and up to the implementation. In particular, when our strtok_r returns the final token, it sets *last to NULL, but the application needs to call strtok_r one more time to see the NULL *return* value. This means the final call to strtok_r for an application iterating over the tokens is going to dereference a null pointer. See https://godbolt.org/z/9qEqb34z4 for an example proving the old code works but your new code renders the standard use broken.

Also, strtok_r is POSIX not C.

Don't think of it in terms of changes, but think of the code how it IS.

Especially when the other code has dst FIRST and then src. Consistency is better even in small cases like this

unsigned char is always a byte per the c standard. Strangely, it is not guaranteed for char, despite it being that way on pretty much every arch. It feels wrong to operate on the char when the function is about raw memory, not string chars.

Also reason: tloop1 is not defined when tloop is, so there is no guarantee of it being properly substituted, even though every compiler thus far has been doing so

Comments still apply; zero reason to change the char to u_char below. Then you don't need to include a new header too.

Old code was clearer

style(9) violation

is char * always one byte? if so then you are right

sizeof(char) == sizeof(signed char) == sizeof(unsigned char) == 1 by definition

Comma operator? Really?