[WIP Please ignore] Add cli completion ability; implement for esp and nrf #14478
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR #14478: Size comparison from 92f8343 to 43d19df Increases (3 builds for cyw30739, telink)
Decreases (4 builds for cyw30739, linux, telink)
Full report (16 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
|
PR #14478: Size comparison from 92f8343 to 1f9b2f2 Increases (5 builds for cyw30739, esp32, nrfconnect, telink)
Decreases (6 builds for cyw30739, esp32, linux, nrfconnect, telink)
Full report (33 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
pullapprove
bot
requested review from
andy31415,
anush-apple,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs and
kpschoedel
pullapprove
bot
requested review from
LuDuda,
lzgrablic02,
mlepage-google,
msandstedt,
pan-apple,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
andy31415
requested changes
Feb 1, 2022
|
|
||
| public: | ||
| Engine() {} | ||
| Engine(){}; |
There was a problem hiding this comment.
supernit: remove the ; change here and below. There should be no ; after method implementations
| if (strcmp(prefix, "") != 0) | ||
| { | ||
| strncpy(&sub_prefix[pos], prefix, strlen(prefix)); | ||
| pos += strlen(prefix); |
There was a problem hiding this comment.
Just use strcpy:
- strncpy is not using the sub_prefix pos at all and sub_prefix size is preallocated
- using strncpy but then advancing by strlen(prefix) will not make use of strncpy guarantees (we move up to length of prefix anyway)
| { | ||
| strncpy(&sub_prefix[pos], prefix, strlen(prefix)); | ||
| pos += strlen(prefix); | ||
| strncpy(&sub_prefix[pos], " ", 1); |
There was a problem hiding this comment.
strncpy with size 1 is the same like sub_prefix[pos] = ' '; right? And the assignment makes it clear that the string is no longer guaranteed to be null terminated (which strncpy above does not guarantee either). Does this need fix?
| strncpy(&sub_prefix[pos], " ", 1); | ||
| pos += 1; | ||
| } | ||
| strncpy(&sub_prefix[pos], syntax, strlen(syntax) + 1); |
There was a problem hiding this comment.
using strlen of the 2nd argument in strncpy negates the usefulness of strncpy. Just use strcpy to make it clear what it does and that we do not validate sizes at all.
| const struct shell_static_entry * build_command_tree(const char * prefix, const char * syntax, const char * help) | ||
| { | ||
|
|
||
| char * sub_prefix = new char[strlen(prefix) + strlen(syntax) + 2]; |
There was a problem hiding this comment.
use ScopedMemoryBuffer here. Otherwise tracking if sub_prefix is allocated and freed correctly is hard.
Also add a guard to see if memory allocation succeeded.
| memcpy((shell_static_entry *) &sub_static_entry[i], &leaf_entry, sizeof(struct shell_static_entry)); | ||
| continue; | ||
| } | ||
| memcpy((shell_static_entry *) &sub_static_entry[i], sub_entry, sizeof(struct shell_static_entry)); |
There was a problem hiding this comment.
why is the cast required here/above/below?
| // Zephyr's shell_cmd_entry.u.entry needs an additional NULL element to terminate the array. | ||
| memset((shell_static_entry *) &sub_static_entry[context.cmdc], 0, sizeof(struct shell_static_entry)); | ||
|
|
||
| const struct shell_cmd_entry * sub_cmd_entry = |
There was a problem hiding this comment.
lifetime comments for these and below.
Also explanation of why this does not use PlatformAlloc but direct new
| if (buf[len - 1] == ' ') | ||
| { | ||
| trailing_space = true; | ||
| memset((char *) &buf[len - 1], 0, 1); |
There was a problem hiding this comment.
this is a very complex way of saying buf[len-1] = 0. Please replace with simple assignment
| { | ||
| // remove "matter " prefix for completion lookup as the command | ||
| // registeration in general are done without "matter" prefix | ||
| char * line = new char[len - strlen(matter_prefix) + trailing_space + 1]; |
There was a problem hiding this comment.
Please don't use bools in integer arithmentic. Feels like "tricky code" even if it works.
There was a problem hiding this comment.
Use scoped buffers for line, check allocation.
|
|
||
| // Write "matter " | ||
| size_t pos = 0; | ||
| strncpy(&cmd_completion[pos], matter_prefix, strlen(matter_prefix) + 1); |
There was a problem hiding this comment.
strncpy should take the target destination as size or not take any size at all. Use strcpy if you cannot use the size of destination, since strcpy is supposed to protect against destination overflow.
There was a problem hiding this comment.
In hindsight after seeing all this code: you should probably be using src/lib/support/StringBuilder.h to have these append logic work and then you do not need to worry as much about pos and sizes and such.
StringBuilderBase with a ScopedBuffer for memory allocation should work well.
There was a problem hiding this comment.
thank you very much for taking time to review and the guidance, really appreciate it. Let me work on the changes to address them and will send this again later.
lmpprk
changed the title
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Change overview
Engine::GetCmdCompletionto provide command completion based on command prefixesPreviously add CLI tab-to-autocomplete ability and implement for nrfconnect #13630 reverted due to racing changes with other command additions.
Testing
Build and tested manually with
nRF52840dk with lighting-app example
esp32c3 with all-clusters-app example
