aws / Jobs-for-AWS-IoT-embedded-sdk

Client library for using AWS IoT Jobs service on embedded devices
MIT License
13 stars 38 forks source link

Performance and Safety Improvements #93

Closed BogdanTheGeek closed 9 months ago

BogdanTheGeek commented 9 months ago

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.

bradleysmith23 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

/bot run formatting

kstribrnAmzn commented 9 months ago

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.