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

1-refactor-v1.0.1 #35

Merged
merged 18 commits into from
Dec 21, 2021
Merged

1-refactor-v1.0.1 #35

merged 18 commits into from
Dec 21, 2021

Conversation

jlucas9
Copy link
Collaborator

@jlucas9 jlucas9 commented Dec 17, 2021

I did touch every single file, but ensured that all existing tests passed at each step along the way.
Really just a lot of reformatting with the last two commits being the largest and potentially controversial as it exposes additional functionality to user space in favor of smaller files.

Let me know what you think - if you'd rather not have one of these changes or if something was missed!
Thanks in advance.

@@ -68,6 +68,10 @@ int32_t Crypto_TC_ApplySecurity(const uint8_t *p_in_frame, const uint16_t in_fra
printf("%02X", ((uint8_t *)&*p_in_frame)[i]);
}
printf("\nPrinted %d bytes\n", in_frame_length);
#else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can also do: (void) in_frame_length;
This method is used in FreeRTOS a lot.

@IbraheemYSaleh
Copy link
Contributor

There're definitely some style clashes here that I don't fully agree with (EG, enforcing the 120 maximum characters per a single line rule, forcing even simple if blocks to span multiple lines, etc), but overall, this refactor does improve the code structure for CryptoLib 👍 ... The only real issue I have is that the MariaDB code is broken with the build (-DMYSQL=ON) ... I'll go ahead and start working on fixing that and push the commits to this branch.

Copy link
Contributor

@IbraheemYSaleh IbraheemYSaleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my changes (the last commit), the MariaDB code is working again and passing our downstream unit tests. I approve of this refactor now 👍 ... Please merge when ready.

src/src_main/sadb_routine_inmemory.template.c Outdated Show resolved Hide resolved
sa->akid = atoi(row[i]);
continue;
}
if (strcmp(field_names[i], "sa_state") == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment (no code changes expected here), I've always been under the impression that code readability trumps style consistency. Having these blocks tightly packed on ~30 single, easy to read lines was, in my opinion, infinitely more readable/valuable than having them spread over these 150+ lines.

#else
// TODO - Find another way to know this and remove this argument
uint16_t tmp = in_frame_length;
tmp = tmp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the purpose of this 'tmp' variable... Or the tmp = tmp statement.

src/src_main/crypto.c Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@rjbrown2 rjbrown2 merged commit 0467e37 into collab_main Dec 21, 2021
@jlucas9 jlucas9 mentioned this pull request Jan 3, 2022
@rjbrown2 rjbrown2 deleted the 1-refactor-v1.0.1 branch January 13, 2022 18:27
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

Successfully merging this pull request may close these issues.

5 participants