Skip to content

Conversation

@ndrs-pst
Copy link
Contributor

@ndrs-pst ndrs-pst commented Nov 7, 2024

Add len to store the result of strlen(addr_arg) to avoid multiple calls to strlen within the for-loop in cmd_scan_filter_set_addr.
While the performance impact may be minimal in a shell context, storing strlen(addr_arg) in len ensures a single call, making the code more predictable and consistent.

@Thalley Thalley added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Nov 7, 2024
Copy link
Contributor

@alwa-nordic alwa-nordic left a comment

Choose a reason for hiding this comment

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

Looks good. Just a comment on the commit message: The commit message does not say why it's good to avoid multiple calls to strlen. Can you give your reason? Is it performance, or readability? I assume it's the latter. But it's better when the contributor documents the thinking that went into a change. :)

@ndrs-pst
Copy link
Contributor Author

ndrs-pst commented Nov 8, 2024

Thank you for the feedback!
I'll update the commit message to highlight the consistency aspect.
While runtime impact may be minimal in a shell context, this pattern appears in several places in the Zephyr codebase, with an assumption that strlen would only be called once and stored optimistically by the compiler. By storing it in len, we ensure a single strlen call, making the code more predictable.

Add `len` to store the result of `strlen(addr_arg)` to avoid
multiple calls to `strlen` within the `for-loop` in
`cmd_scan_filter_set_addr`.
While the performance impact may be minimal in a shell context,
storing `strlen(addr_arg)` in `len` ensures a single call,
making the code more predictable and consistent.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
@ndrs-pst ndrs-pst force-pushed the pr_bt_shell_avoid_multiple_strlen_call branch from 44a77e7 to da9ab10 Compare November 8, 2024 09:16
@ndrs-pst
Copy link
Contributor Author

ndrs-pst commented Nov 8, 2024

@alwa-nordic, Oh BTW, it looks like @Thalley wants some feedback from you on this PR #74652 😃

@nashif nashif merged commit c120ffb into zephyrproject-rtos:main Nov 16, 2024
27 checks passed
@ndrs-pst ndrs-pst deleted the pr_bt_shell_avoid_multiple_strlen_call branch November 17, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants