Wdeclaration-after-statement and clang-tidy fixes#291
Wdeclaration-after-statement and clang-tidy fixes#291Frauschi wants to merge 3 commits intowolfSSL:mainfrom
Conversation
Frauschi
force-pushed
the
declaration-after-statement
branch
3 times, most recently
from
192e45e to
b714d77
Compare
|
Currently, |
Frauschi
changed the title
Frauschi
force-pushed
the
declaration-after-statement
branch
from
b714d77 to
489c792
Compare
|
Hmmm I have no problems with this PR but I am not sure we needed this. wolfHSM != wolfSSL. wolfHSM is even talking about moving to C11 compatibility on our end. Every compiler for our platform ports supports C11 so silly things like "declare all variables up top" to support some compiler from the 90s don't apply to us. FYI @JacobBarthelmeh. Open to discussion though. |
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
@Frauschi I appreciate this change but massive AI-assisted refactors like this are basically unreviewable. I would really prefer in the future that you go on a module-by-module basis so it can be sanely reviewed. See below for some of the subtle bugs introduced that are basically impossible to spot without scrutiny. Please break refactors like this down into smaller more reviewable chunks in the future, ESPECIALLY if they are largely AI-driven.
| char* end; | ||
| errno = 0; | ||
| unsigned long val = strtoul(argv[i + 1], &end, 0); | ||
| errno = 0; |
There was a problem hiding this comment.
you clear errno here right before the error check? Needs to be cleared before strtoul like in the original code
| uint32_t *out_reclaim_size, whNvmId *out_reclaim_objects) | ||
| { | ||
| whNvmFlashContext* context = c; | ||
| nfMemDirectory *d = &context->directory; |
There was a problem hiding this comment.
you dereference context->directory vefore the null check on context
| WC_RNG rng[1]; | ||
| uint8_t buffer[128] = {0}; | ||
|
|
||
| #if !defined(WOLFHSM_CFG_NO_CRYPTO) |
There was a problem hiding this comment.
relocating this with to the top improperly macros out the arg parsing now, meaning building without nocrypto effectively skips all of main
| whMessageCrypto_CmacAesDmaRequest req; | ||
| whMessageCrypto_CmacAesDmaResponse res; | ||
| void* inAddr = NULL; | ||
| uint8_t tmpKey[AES_256_KEY_SIZE]; |
There was a problem hiding this comment.
why did we change this to AES_256_KEY_SIZE from AES_MAX_KEY_SIZE?
| whMessageCrypto_EccVerifyRequest req; | ||
| whMessageCrypto_EccVerifyResponse res; | ||
|
|
||
| uint32_t available = inSize - sizeof(whMessageCrypto_EccVerifyRequest); |
There was a problem hiding this comment.
I'd prefer available be assigned and used after inSize is validated. This was the original pattern, and is still used in CMAC, however this got changed in a bunch of other functions like this one
3 participants
As requested by @JacobBarthelmeh, this PR fixes the code base to ensure no variable declarations are done after statements.
For that, the tests, benchmark and examples are all compiled with
-Wdeclaration-after-statementnow. Hence, all future code will also ensure that this is ensured.Furthermore, a lot of warnings uncovered by clang-tidy in CI are fixed.