Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe you want replace INT to UINT8_T here. #225

Open
pavelkolodin opened this issue May 1, 2024 · 2 comments
Open

Maybe you want replace INT to UINT8_T here. #225

pavelkolodin opened this issue May 1, 2024 · 2 comments

Comments

@pavelkolodin
Copy link

The variable i here can be uint8_t too.

int i;

@Ingmar-Paetzold
Copy link

Ingmar-Paetzold commented Feb 16, 2025

You are right for consistency reasons. The Code uses several types for the for-loop indexes: int, unsigned, size_t and uint8_t. These should all be the same.

I am personally a big fan of Stroustrup's guidelines, which state that unsigned should only be used for bit manipulation, which are all the arrays here that are part of the AES calculation.
And signed should be used for arithmetic. It's personal taste, and I found that it makes a lot of sense in most cases.
See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es101-use-unsigned-types-for-bit-manipulation and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es102-use-signed-types-for-arithmetic

At least, it does not make sense to use a fixed-bit type like uint8_t. Because the for loop indexes are counters, not tied to a fixed bit number. So make it portable and let the compiler figure out the optimum.

-> Therefore,I would go with int for all for loops here.

@Nable80
Copy link

Nable80 commented Feb 16, 2025

At least, it does not make sense to use a fixed-bit type like uint8_t. Because the for loop indexes are counters, not tied to a fixed bit number. So make it portable and let the compiler figure out the optimum.

Unfortunately, it does make sense in this case. Standard C suc^W isn't well suitable for 8-bit machines, while this library's primary target is AVR and similar MCU architectures, where compilers fail to figure out when it's possible to use 8-bit integers instead of 16+-bit ones. I guess it's possible to improve this situation with typedef/define and multiple ifdef's in order to define counters as unsigned or even size_t (IMHO, Stroustrup doesn't care about hardware enough) on larger platforms, while keeping the code compact and fast on 8-bit ones… but someone should make and send a PR with this change that won't break/worsen existing use-cases. That's definitely possible but it requires some work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants