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

Performance and Safety Improvements #93

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

BogdanTheGeek
Copy link
Contributor

This PR includes 3 changes I consider important. They are well separated so you can cherrypick out commits that you don't consider beneficial. I am happy to make any changes that help with code conformity.

Change 1:
Define arrays as static to avoid copy to stack.

Change 2:
Use Macro function for string literal length calculation, avoids off by 1 errors.

Change 3:
Introduce asserts for function parameters that can overflow. Also included static_assert definitions to help with co-dependent array declarations.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@BogdanTheGeek BogdanTheGeek changed the title Performance and Safety Imporvements Performance and Safety Improvements Dec 15, 2023
@bradleysmith23 bradleysmith23 self-requested a review December 15, 2023 18:43
@bradleysmith23
Copy link
Contributor

Change 1: Define arrays as static to avoid copy to stack.

Change 2: Use Macro function for string literal length calculation, avoids off by 1 errors.

These changes would both be welcome! Thank you raising the PR.

Change 3:
Introduce asserts for function parameters that can overflow. Also included static_assert definitions to help with co-dependent array declarations.

As for this change, the use of static_assert() moves too far away from C90. Please update this to solely use assert(), at which point I will review and approve these changes.
This should also fix the issues for the unit tests failing in the CI. For the formatting and memory_statistics checks, they can be quickly addressed once this change is made. Thank you again!

@BogdanTheGeek
Copy link
Contributor Author

As for this change, the use of static_assert() moves too far away from C90. Please update this to solely use assert(), at which point I will review and approve these changes. This should also fix the issues for the unit tests failing in the CI. For the formatting and memory_statistics checks, they can be quickly addressed once this change is made. Thank you again!

I had provided a common alternative to static_assert() in the macro deffiniton for anyone stuck with a 33yo compiler. Static asserts provide better diagnostics than runtime asserts. I would advocate for this addition, especially because the alternative implementation still works for any ANSI C compiler. However, I fully expected this, which is why i split the commits to begin with.

I have removed the static asserts.

@BogdanTheGeek
Copy link
Contributor Author

I see why the static_assert failed, the error string had spaces in it. Regardless, the latest commit has no static asserts so they are ready for review.
Thank you.

@bradleysmith23
Copy link
Contributor

/bot run formatting

Copy link
Contributor

@bradleysmith23 bradleysmith23 left a comment

Choose a reason for hiding this comment

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

Approved! Thank you for submitting this PR and making the requested changes.

@kstribrnAmzn
Copy link
Member

for anyone stuck with a 33yo compiler

Trust me - sometimes we feel that deeply as well. Backwards compatibility - while extremely important - can be a pain when new features simplify your life.

@kstribrnAmzn kstribrnAmzn merged commit e4a4ef3 into aws:main Dec 21, 2023
10 checks passed
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.

4 participants