Skip to content

Conversation

@ndrs-pst
Copy link
Contributor

@ndrs-pst ndrs-pst commented Jul 31, 2024

Streamline wifi_shell.c with following commits:

  • Code formatting to improve readability
  • Apply remaining struct option as static const to reduce .data area usage

Since the rebase was applied after PR #73707 which led to two more commits in this PR 😅

  • Use getopt_state for safer optarg access
  • Enhance consistency in code style

jukkar
jukkar previously approved these changes Jul 31, 2024
rlubos
rlubos previously approved these changes Jul 31, 2024
@jukkar
Copy link
Member

jukkar commented Aug 7, 2024

@ndrs-pst can you check the CI issue so we get this merged

krish2718
krish2718 previously approved these changes Aug 7, 2024
Various manual code formatting adjustments, including:
- Wrapping lines in the `long_options` declaration to prevent them
  from extending too far to the right.
- Adding missing `{` and `}` in the `if` statement in
  `cmd_wifi_set_rts_threshold`.
- Aligning `SHELL_CMD_ARG` in `wifi_commands` with previous declarations.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
This change marks the remaining instance of the `struct option` as
`static const`.

The rationale is that `struct option` is a read-only variable.
By using `static const`, we ensure immutability, leading to usage of only
the `.rodata` section and a reduction in the `.data` area.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
@ndrs-pst ndrs-pst dismissed stale reviews from krish2718, rlubos, and jukkar via 9fb5b2f August 8, 2024 04:56
@ndrs-pst ndrs-pst force-pushed the pr_net_wifi_shell_streamline_codebase branch from 7b558d0 to 9fb5b2f Compare August 8, 2024 04:56
Using `getopt_state` to access `optarg` and also `optopt` offers
a better alternative to direct global access.

See e145eb9 for the previous change related to this.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
@ndrs-pst ndrs-pst force-pushed the pr_net_wifi_shell_streamline_codebase branch from 9fb5b2f to 63a6013 Compare August 8, 2024 05:13
@ndrs-pst
Copy link
Contributor Author

ndrs-pst commented Aug 8, 2024

@ndrs-pst can you check the CI issue so we get this merged

Rebase and resolve merge conflicts, incorporating two additional commits.
Apologies for any inconvenience caused.

Copy link
Member

Choose a reason for hiding this comment

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

I thought that the rule was opposite i.e., to have spaces there, at least it was like that in the past, but perhaps this has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW after dumping with following command since Zephyr use BasedOnStyle: LLVM

clang-format -style=llvm -dump-config > .clang-format-llvm

Seems this configuration was enabled Cpp11BracedListStyle: true
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Copy link
Member

Choose a reason for hiding this comment

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

There are many styles possible, not sure this one is the best for us.

Could you take this commit away from this PR, lets figure out the used style later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's follow existing style in Zephyr, if needed a separate PR that fixes style in enitre repo can be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jukkar Rather than take this commit away, how about to remove just the following?

  • Reformatted { 0 } to {0} as per .clang-format settings for uniformity.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that is fine

Enhancing code consistency provides cognitive leverage with
the following changes:
- Unified the order of declaration for `opt`, `opt_index`,
  `state`, and `long_options`.
- Unified the wrapping of `getopt_long` calls, regardless of
  the length of the `options` string.
- Renamed `option_index` to `opt_index` for consistency.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
@ndrs-pst ndrs-pst force-pushed the pr_net_wifi_shell_streamline_codebase branch from 63a6013 to d6f2f43 Compare August 8, 2024 10:33
@cfriedt cfriedt merged commit 721ee31 into zephyrproject-rtos:main Aug 9, 2024
@ndrs-pst ndrs-pst deleted the pr_net_wifi_shell_streamline_codebase branch August 9, 2024 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants