Hello All!
Here are a few code cleanups after Jean's review:
[PATCH 1/3] Documentation/kconfig: more concise and straightforward
[PATCH 2/3] kconfig: avoid multiple calls to strlen
[PATCH 3/3] kconfig: cleanup symbol-search code
If everything is good, I'll send a pull-request to Michal a bit later
during the rc phase (in case more fixups accumulate).
Regards,
Yann E. MORIN.
From: "Yann E. MORIN" <[email protected]>
Calls to strlen are costly, so avoid calling strln as much as we can.
Reported-by: Jean Delvare <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
Cc: Jean Delvare <[email protected]>
---
scripts/kconfig/symbol.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index d550300..020a0ac 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -967,7 +967,7 @@ static int sym_rel_comp( const void *sym1, const void *sym2 )
{
struct sym_match *s1 = *(struct sym_match **)sym1;
struct sym_match *s2 = *(struct sym_match **)sym2;
- int l1, l2;
+ int exact1, exact2;
/* Exact match:
* - if matched length on symbol s1 is the length of that symbol,
@@ -978,11 +978,11 @@ static int sym_rel_comp( const void *sym1, const void *sym2 )
* exactly; if this is the case, we can't decide which comes first,
* and we fallback to sorting alphabetically.
*/
- l1 = s1->eo - s1->so;
- l2 = s2->eo - s2->so;
- if (l1 == strlen(s1->sym->name) && l2 != strlen(s2->sym->name))
+ exact1 = (s1->eo - s1->so) == strlen(s1->sym->name);
+ exact2 = (s2->eo - s2->so) == strlen(s2->sym->name);
+ if (exact1 && !exact2)
return -1;
- if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
+ if (!exact1 && exact2)
return 1;
/* As a fallback, sort symbols alphabetically */
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
- no need for a double-indirection for the temporary sym_match_arr array
- the temporary sym_match_arr array is not NULL terminated, so no need
to allocate n+1 elements
- two minor style fixes
- grammar fix in comment
Reported-by: Jean Delvare <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
Cc: Jean Delvare <[email protected]>
---
scripts/kconfig/symbol.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 020a0ac..a76b8fd 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -963,10 +963,10 @@ struct sym_match {
* - first, symbols that match exactly
* - then, alphabetical sort
*/
-static int sym_rel_comp( const void *sym1, const void *sym2 )
+static int sym_rel_comp(const void *sym1, const void *sym2)
{
- struct sym_match *s1 = *(struct sym_match **)sym1;
- struct sym_match *s2 = *(struct sym_match **)sym2;
+ const struct sym_match *s1 = sym1;
+ const struct sym_match *s2 = sym2;
int exact1, exact2;
/* Exact match:
@@ -992,7 +992,7 @@ static int sym_rel_comp( const void *sym1, const void *sym2 )
struct symbol **sym_re_search(const char *pattern)
{
struct symbol *sym, **sym_arr = NULL;
- struct sym_match **sym_match_arr = NULL;
+ struct sym_match *sym_match_arr = NULL;
int i, cnt, size;
regex_t re;
regmatch_t match[1];
@@ -1005,47 +1005,38 @@ struct symbol **sym_re_search(const char *pattern)
return NULL;
for_all_symbols(i, sym) {
- struct sym_match *tmp_sym_match;
if (sym->flags & SYMBOL_CONST || !sym->name)
continue;
if (regexec(&re, sym->name, 1, match, 0))
continue;
- if (cnt + 1 >= size) {
+ if (cnt >= size) {
void *tmp;
size += 16;
- tmp = realloc(sym_match_arr, size * sizeof(struct sym_match *));
- if (!tmp) {
+ tmp = realloc(sym_match_arr, size * sizeof(struct sym_match));
+ if (!tmp)
goto sym_re_search_free;
- }
sym_match_arr = tmp;
}
sym_calc_value(sym);
- tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
- if (!tmp_sym_match)
- goto sym_re_search_free;
- tmp_sym_match->sym = sym;
- /* As regexec return 0, we know we have a match, so
+ /* As regexec returned 0, we know we have a match, so
* we can use match[0].rm_[se]o without further checks
*/
- tmp_sym_match->so = match[0].rm_so;
- tmp_sym_match->eo = match[0].rm_eo;
- sym_match_arr[cnt++] = tmp_sym_match;
+ sym_match_arr[cnt].so = match[0].rm_so;
+ sym_match_arr[cnt].eo = match[0].rm_eo;
+ sym_match_arr[cnt++].sym = sym;
}
if (sym_match_arr) {
- qsort(sym_match_arr, cnt, sizeof(struct sym_match*), sym_rel_comp);
+ qsort(sym_match_arr, cnt, sizeof(struct sym_match), sym_rel_comp);
sym_arr = malloc((cnt+1) * sizeof(struct symbol));
if (!sym_arr)
goto sym_re_search_free;
for (i = 0; i < cnt; i++)
- sym_arr[i] = sym_match_arr[i]->sym;
+ sym_arr[i] = sym_match_arr[i].sym;
sym_arr[cnt] = NULL;
}
sym_re_search_free:
- if (sym_match_arr) {
- for (i = 0; i < cnt; i++)
- free(sym_match_arr[i]);
- free(sym_match_arr);
- }
+ /* sym_match_arr can be NULL if no match, but free(NULL) is OK */
+ free(sym_match_arr);
regfree(&re);
return sym_arr;
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
Re-phrase the explanations on the sorting of search results, in a more
concise and complete way.
Drop reference to the user's locale when sorting alphabetically, since
this is implicit.
Reported-by: Jean Delvare <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
Cc: Jean Delvare <[email protected]>
---
Documentation/kbuild/kconfig.txt | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
index e9f9e76..6edf37d 100644
--- a/Documentation/kbuild/kconfig.txt
+++ b/Documentation/kbuild/kconfig.txt
@@ -175,11 +175,9 @@ Searching in menuconfig:
/^hotplug
When searching, symbols are sorted thus:
- - exact match first: an exact match is when the search matches
- the complete symbol name;
- - alphabetical order: when two symbols do not match exactly,
- they are sorted in alphabetical order (in the user's current
- locale).
+ - first, exact matches, sorted alphabetically (an exact match │18:03 -!- - keep a notice in the topic and perhaps as an on-join message).
+ is when the search matches the complete symbol name); │18:03 -!- -
+ - then, other matches, sorted alphabetically.
For example: ^ATH.K matches:
ATH5K ATH9K ATH5K_AHB ATH5K_DEBUG [...] ATH6KL ATH6KL_DEBUG
[...] ATH9K_AHB ATH9K_BTCOEX_SUPPORT ATH9K_COMMON [...]
--
1.8.1.2
All,
On 2013-07-13 20:08 +0200, Yann E. MORIN spake thusly:
> From: "Yann E. MORIN" <[email protected]>
>
> Re-phrase the explanations on the sorting of search results, in a more
> concise and complete way.
>
> Drop reference to the user's locale when sorting alphabetically, since
> this is implicit.
>
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
> Cc: Jean Delvare <[email protected]>
> ---
> Documentation/kbuild/kconfig.txt | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
> index e9f9e76..6edf37d 100644
> --- a/Documentation/kbuild/kconfig.txt
> +++ b/Documentation/kbuild/kconfig.txt
> @@ -175,11 +175,9 @@ Searching in menuconfig:
> /^hotplug
>
> When searching, symbols are sorted thus:
> - - exact match first: an exact match is when the search matches
> - the complete symbol name;
> - - alphabetical order: when two symbols do not match exactly,
> - they are sorted in alphabetical order (in the user's current
> - locale).
> + - first, exact matches, sorted alphabetically (an exact match │18:03 -!- - keep a notice in the topic and perhaps as an on-join message).
> + is when the search matches the complete symbol name); │18:03 -!- -
> + - then, other matches, sorted alphabetically.
Woops, forget this one, it appears I've busted the copy-paste, and
it was too far right on my terminal. I only saw now, as my mailer
wraps long lines.
Sigh... :-(
Sorry,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <[email protected]>
>
> Re-phrase the explanations on the sorting of search results, in a more
> concise and complete way.
>
> Drop reference to the user's locale when sorting alphabetically, since
> this is implicit.
>
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
> Cc: Jean Delvare <[email protected]>
> ---
> Documentation/kbuild/kconfig.txt | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
> index e9f9e76..6edf37d 100644
> --- a/Documentation/kbuild/kconfig.txt
> +++ b/Documentation/kbuild/kconfig.txt
> @@ -175,11 +175,9 @@ Searching in menuconfig:
> /^hotplug
>
> When searching, symbols are sorted thus:
> - - exact match first: an exact match is when the search matches
> - the complete symbol name;
> - - alphabetical order: when two symbols do not match exactly,
> - they are sorted in alphabetical order (in the user's current
> - locale).
> + - first, exact matches, sorted alphabetically (an exact match │18:03 -!- - keep a notice in the topic and perhaps as an on-join message).
> + is when the search matches the complete symbol name); │18:03 -!- -
> + - then, other matches, sorted alphabetically.
> For example: ^ATH.K matches:
> ATH5K ATH9K ATH5K_AHB ATH5K_DEBUG [...] ATH6KL ATH6KL_DEBUG
> [...] ATH9K_AHB ATH9K_BTCOEX_SUPPORT ATH9K_COMMON [...]
After fixing the copy-and-paste mistake, this looks good to me.
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
Suse L3
Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <[email protected]>
>
> Calls to strlen are costly, so avoid calling strln as much as we can.
Typo: strln -> strlen.
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
> Cc: Jean Delvare <[email protected]>
> ---
> scripts/kconfig/symbol.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index d550300..020a0ac 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -967,7 +967,7 @@ static int sym_rel_comp( const void *sym1, const void *sym2 )
> {
> struct sym_match *s1 = *(struct sym_match **)sym1;
> struct sym_match *s2 = *(struct sym_match **)sym2;
> - int l1, l2;
> + int exact1, exact2;
>
> /* Exact match:
> * - if matched length on symbol s1 is the length of that symbol,
> @@ -978,11 +978,11 @@ static int sym_rel_comp( const void *sym1, const void *sym2 )
> * exactly; if this is the case, we can't decide which comes first,
> * and we fallback to sorting alphabetically.
> */
> - l1 = s1->eo - s1->so;
> - l2 = s2->eo - s2->so;
> - if (l1 == strlen(s1->sym->name) && l2 != strlen(s2->sym->name))
> + exact1 = (s1->eo - s1->so) == strlen(s1->sym->name);
> + exact2 = (s2->eo - s2->so) == strlen(s2->sym->name);
> + if (exact1 && !exact2)
> return -1;
> - if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
> + if (!exact1 && exact2)
> return 1;
>
> /* As a fallback, sort symbols alphabetically */
Looks good and tested OK.
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
Suse L3
Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <[email protected]>
>
> - no need for a double-indirection for the temporary sym_match_arr array
> - the temporary sym_match_arr array is not NULL terminated, so no need
> to allocate n+1 elements
> - two minor style fixes
> - grammar fix in comment
>
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
> Cc: Jean Delvare <[email protected]>
> ---
> scripts/kconfig/symbol.c | 39 +++++++++++++++------------------------
> 1 file changed, 15 insertions(+), 24 deletions(-)
> (...)
You mixed style cleanups with actual code changes, which is usually
avoided. Michal may ask you to split these changes into two separate
patches.
Still, I'm happy with all the changes:
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
Suse L3
Jean, All,
On Tuesday 16 July 2013 16:33:47 Jean Delvare wrote:
> Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> > From: "Yann E. MORIN" <[email protected]>
> >
> > - no need for a double-indirection for the temporary sym_match_arr array
> > - the temporary sym_match_arr array is not NULL terminated, so no need
> > to allocate n+1 elements
> > - two minor style fixes
> > - grammar fix in comment
> >
> > Reported-by: Jean Delvare <[email protected]>
> > Signed-off-by: "Yann E. MORIN" <[email protected]>
> > Cc: Jean Delvare <[email protected]>
> > ---
> > scripts/kconfig/symbol.c | 39 +++++++++++++++------------------------
> > 1 file changed, 15 insertions(+), 24 deletions(-)
> > (...)
>
> You mixed style cleanups with actual code changes, which is usually
> avoided. Michal may ask you to split these changes into two separate
> patches.
Yes, I initially did three patches (style cleanups, n+1 elem, then
double-indirection), and I eventually decided to merge them in a single
patch. I don't know why I decided that... :-(
I'll re-split.
> Still, I'm happy with all the changes:
>
> Reviewed-by: Jean Delvare <[email protected]>
Thank you! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software Designer | \ / CAMPAIGN | ^ |
| --==< O_o >==-- '------------.-------: X AGAINST | /e\ There is no |
| http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL | """ conspiracy. |
'------------------------------'-------'------------------'--------------------'