Modern C approach to memswap after we found issues on ARM architectures with…

Modern C approach to memswap after we found issues on ARM architectures with some compilers with the old version.

Waiting for ARM to confirm if this makes EAGLE_6 run.

changed the description

Yeah, that makes a lot more sense than my implementation... Thanks for fixing this!

LGTM, @pdraper do you want to sanity-check and merge this?

Sadly this still doesn't fix #501. I will commit further but any final version will be based on this so we can test this version and check it does work correctly on current architectures.

added 1 commit

• 2353133c - restricted the pointers in memswap as the two pointers overlapping is undefined…

Compare with previous version

assigned to @pdraper

added 1 commit

• b7561072 - Promoted to int_least8_t just in case. This will also match other C11 types…

Compare with previous version

changed the description

unmarked as a Work In Progress

@pdraper resolved the WIP

Do you have a corner case that comes to mind that we should test?

I don't think so - this doesn't actually change very much, mostly just standardises the code. It will still break with clang I expect.

Only question I have is why use the least types? Doesn't this code really require the boundaries of the fixed sized types for efficiency?

Just in case things aren't defined (C11 requires the _least versions but not the int32_t type). I think it will likely never make a difference on an architecture we run on at current though. The least types will just give us the same types on all x86/ARM architectures i know

Thanks, I see, it seems contradictory that the fixed width ints are optional, but these others aren't, but indeed that is true for C. I think POSIX requires these up to the 32bit size, but the 64bit ones are only required if available (see the stdint.h man page, that is a grab of the standard).

Never worried if we are portable to non-POSIX systems. Anyway happy to leave as it is.

OK, going to accept if we're all happy?

I am happy. :)

merged

mentioned in commit e6517e6d

Done.