[PATCH 0/3] Misc cleanups in kconfig for symbol-search

[PATCH 0/3] Misc cleanups in kconfig for symbol-search

[Yann E. MORIN]

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.

[PATCH 1/3] Documentation/kconfig: more concise and straightforward search explanation

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

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 <jdelvare@suse.de>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Jean Delvare <jdelvare@suse.de>
---
 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

Re: [PATCH 1/3] Documentation/kconfig: more concise and straightforward search explanation

[Yann E. MORIN]

All,

On 2013-07-13 20:08 +0200, Yann E. MORIN spake thusly:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> 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 <jdelvare@suse.de>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Jean Delvare <jdelvare@suse.de>
> ---
>  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.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH 1/3] Documentation/kconfig: more concise and straightforward search explanation

[Jean Delvare]

Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> 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 <jdelvare@suse.de>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Jean Delvare <jdelvare@suse.de>
> ---
>  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 <jdelvare@suse.de>

-- 
Jean Delvare
Suse L3

[PATCH 2/3] kconfig: avoid multiple calls to strlen

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Calls to strlen are costly, so avoid calling strln as much as we can.

Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Jean Delvare <jdelvare@suse.de>
---
 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

Re: [PATCH 2/3] kconfig: avoid multiple calls to strlen

[Jean Delvare]

Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> Calls to strlen are costly, so avoid calling strln as much as we can.

Typo: strln -> strlen.

> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Jean Delvare <jdelvare@suse.de>
> ---
>  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 <jdelvare@suse.de>

-- 
Jean Delvare
Suse L3

[PATCH 3/3] kconfig: cleanup symbol-search code

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

- 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 <jdelvare@suse.de>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Jean Delvare <jdelvare@suse.de>
---
 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

Re: [PATCH 3/3] kconfig: cleanup symbol-search code

[Jean Delvare]

Le Saturday 13 July 2013 à 20:08 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> - 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 <jdelvare@suse.de>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Jean Delvare <jdelvare@suse.de>
> ---
>  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 <jdelvare@suse.de>

-- 
Jean Delvare
Suse L3

Re: [PATCH 3/3] kconfig: cleanup symbol-search code

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
> > 
> > - 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 <jdelvare@suse.de>
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Jean Delvare <jdelvare@suse.de>
> > ---
> >  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 <jdelvare@suse.de>

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.  |
'------------------------------'-------'------------------'--------------------'