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

Don't use sprintf #3

Merged
merged 1 commit into from Jan 7, 2021
Merged

Don't use sprintf #3

merged 1 commit into from Jan 7, 2021

Conversation

zauguin
Copy link
Contributor

@zauguin zauguin commented Jan 2, 2021

The code uses sprintf(..., "%s", ...); to copy a string and then directly calculates the length of this string. This is just ridiculously inefficient: sprintf returns the number of bytes anyway, so the return value could be used instead of using strlen, but on a much more fundamental note sprintf is way more powerful than required for such a usecase. It has to parse the format string which is both relatively slow and introduces flash overhead since it happens at runtime and therefore unneeded branches can't be omitted. Additionally it sometimes uses a more flexible but slower memory copy implementation. (Not that that would really matter for upto 32bytes).

Here I used memcpy instead of strcpy since the length has to be calculated anyway. The final NULL byte is not copied since it isn't sent anyway. With my compiler, this reduces the flash size of the firmware by more than 30% (from 51456 bytes to 35496 bytes). Like the original code, it does not check the length and assumes that the version fields never exceed 31 bytes.

}
size_t length = strlen(version);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might look inefficient since APP_VERSIONS is a string literal and therefore the length can be calculated at compile time, but gcc detects this automatically and already uses the constant here.

@scotthsl scotthsl merged commit 8f0ed0f into Snapmaker:main Jan 7, 2021
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.

None yet

2 participants