From: "Yann E. MORIN" <[email protected]>
Hello Michal, All!
│
Michal, please pull these patches against kconfig that I have accumulated
for 3.11.
Note-whorthy this time:
- fix values of tristates that are selected by boolean choices (Arve)
- fix choice randomisation in presence of KCONFIG_ALLCONFIG (me)
- fix choice randomisation selecting more than one value in
a choice (but only if it is conditional) (me)
- fix choice-in-a-choice randomisation not selecting any value
for the inner-most choice (me)
Also, some code-cleanups and eye-candy:
- mconf and nconf code cleanups (Dirk, Sedat)
- mconf and nconf eye-candy (Dirk, me)
- scripts/config script-name in help text (Clément)
- heuristic to sort found symbols by relevance (me)
- more randconfig debugging help (me)
Changes v1 -> v2:
- simplify sorting heuristic (Jean)
- make it clear in mconf/nconf that the search expression can
be a regexp (Jean)
Regards,
Yann E. MORIN.
The following changes since commit f722406faae2d073cc1d01063d1123c35425939e:
Linux 3.10-rc1 (2013-05-11 17:14:08 -0700)
are available in the git repository at:
git://gitorious.org/linux-kconfig/linux-kconfig.git yem-kconfig-for-next
for you to fetch changes up to 8357b48549e17b3e4e402c7f977b65708922e60f:
kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG (2013-06-24 20:03:31 +0200)
----------------------------------------------------------------
Arve Hjønnevåg (1):
kconfig: Fix defconfig when one choice menu selects options that another choice menu depends on
Clement Chauplannaz (1):
scripts/config: replace hard-coded script name by a dynamic value
Dirk Gouders (4):
kconfig/lxdialog: handle newline characters in print_autowrap()
mconf: use function calls instead of ncurses' variables LINES and COLS
nconf: use function calls instead of ncurses' variables LINES and COLS
mconf/nconf: mark empty menus/menuconfigs different from non-empty ones
Sedat Dilek (2):
kconfig/lxdialog: Add definitions for mininimum (re)size values
kconfig/lxdialog: Use new mininimum resize definitions in conf_choice()
Yann E. MORIN (7):
kconfig/conf: fix randconfig setting multiple symbols in a choice
kconfig/conf: accept a base-16 seed for randconfig
kconfig/conf: print the seed used to initialise the RNG for randconfig
kconfig: sort found symbols by relevance
kconfig/[mn]conf: make it explicit in the search box that a regexp is possible
kconfig: loop as long as we changed some symbols in randconfig
kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG
Documentation/kbuild/kconfig.txt | 13 ++++++
scripts/config | 12 +++--
scripts/kconfig/conf.c | 6 ++-
scripts/kconfig/confdata.c | 39 ++++++++++++----
scripts/kconfig/expr.h | 3 ++
scripts/kconfig/lkc.h | 3 +-
scripts/kconfig/lkc_proto.h | 1 +
scripts/kconfig/lxdialog/checklist.c | 8 ++--
scripts/kconfig/lxdialog/dialog.h | 14 ++++++
scripts/kconfig/lxdialog/inputbox.c | 8 ++--
scripts/kconfig/lxdialog/menubox.c | 6 +--
scripts/kconfig/lxdialog/textbox.c | 6 +--
scripts/kconfig/lxdialog/util.c | 46 +++++++++++--------
scripts/kconfig/lxdialog/yesno.c | 8 ++--
scripts/kconfig/mconf.c | 21 +++++----
scripts/kconfig/menu.c | 16 +++++++
scripts/kconfig/nconf.c | 39 +++++++++-------
scripts/kconfig/nconf.gui.c | 20 ++++----
scripts/kconfig/symbol.c | 89 ++++++++++++++++++++++++++++++++----
19 files changed, 260 insertions(+), 98 deletions(-)
--
.-----------------.--------------------.------------------.--------------------.
| 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. |
'------------------------------^-------^------------------^--------------------'
From: "Yann E. MORIN" <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
scripts/kconfig/conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index bde5b95..94521c7 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -527,7 +527,7 @@ int main(int ac, char **av)
seed_env = getenv("KCONFIG_SEED");
if( seed_env && *seed_env ) {
char *endp;
- int tmp = (int)strtol(seed_env, &endp, 10);
+ int tmp = (int)strtol(seed_env, &endp, 0);
if (*endp == '\0') {
seed = tmp;
}
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
Because of choice-in-a-choice constructs, it can happen that not all
symbols are assigned a value during randconfig, leading in rare cases
to this situation:
---8<--- choice-in-choice.in
choice
bool "A/B/C"
config A
bool "A"
config B
bool "B"
if B
choice
bool "E/F"
config E
bool "E"
config F
bool "F"
endchoice
endif # B
config C
bool "C"
endchoice
---8<---
$ ./scripts/kconfig/conf --randconfig choice-in-choice.in
[--SNIP--]
$ ./scripts/kconfig/conf --silentoldconfig choice-in-choice.in </dev/null
[--SNIP--]
A/B/C
1. A (A)
> 2. B (B)
3. C (C)
choice[1-3]: 2
E/F
> 1. E (E) (NEW)
2. F (F) (NEW)
choice[1-2]: aborted!
Console input/output is redirected. Run 'make oldconfig' to update
configuration.
Fix this by looping in randconfig for as long as some symbol gets assigned
a value.
Note: this was spotted with the USB EHCI Debug Device Gadget (USB_G_DBGP),
which uses this choice-in-a-choice construct, and exhibits this problem.
The example above is just a stripped-down minimalist test-case.
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
scripts/kconfig/conf.c | 3 ++-
scripts/kconfig/confdata.c | 18 ++++++++++++++----
scripts/kconfig/lkc.h | 2 +-
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 38616c1..d19944f 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -654,7 +654,8 @@ int main(int ac, char **av)
conf_set_all_new_symbols(def_default);
break;
case randconfig:
- conf_set_all_new_symbols(def_random);
+ /* Really nothing to do in this loop */
+ while (conf_set_all_new_symbols(def_random)) ;
break;
case defconfig:
conf_set_all_new_symbols(def_default);
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index d36bc1f..c55c227 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1040,7 +1040,7 @@ void conf_set_changed_callback(void (*fn)(void))
conf_changed_callback = fn;
}
-static void randomize_choice_values(struct symbol *csym)
+static bool randomize_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
@@ -1053,7 +1053,7 @@ static void randomize_choice_values(struct symbol *csym)
* In both cases stop.
*/
if (csym->curr.tri != yes)
- return;
+ return false;
prop = sym_get_choice_prop(csym);
@@ -1084,6 +1084,8 @@ static void randomize_choice_values(struct symbol *csym)
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
csym->flags &= ~(SYMBOL_VALID);
+
+ return true;
}
void set_all_choice_values(struct symbol *csym)
@@ -1106,7 +1108,7 @@ void set_all_choice_values(struct symbol *csym)
csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
-void conf_set_all_new_symbols(enum conf_def_mode mode)
+bool conf_set_all_new_symbols(enum conf_def_mode mode)
{
struct symbol *sym, *csym;
int i, cnt, pby, pty, ptm; /* pby: probability of boolean = y
@@ -1154,6 +1156,7 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
exit( 1 );
}
}
+ bool has_changed = false;
for_all_symbols(i, sym) {
if (sym_has_value(sym) || (sym->flags & SYMBOL_VALID))
@@ -1161,6 +1164,7 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
switch (sym_get_type(sym)) {
case S_BOOLEAN:
case S_TRISTATE:
+ has_changed = true;
switch (mode) {
case def_yes:
sym->def[S_DEF_USER].tri = yes;
@@ -1219,6 +1223,12 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
- randomize_choice_values(csym);
+ has_changed = randomize_choice_values(csym);
+ else {
+ set_all_choice_values(csym);
+ has_changed = true;
+ }
}
+
+ return has_changed;
}
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index 0c8d419..09f4edf 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -86,7 +86,7 @@ const char *conf_get_autoconfig_name(void);
char *conf_get_default_confname(void);
void sym_set_change_count(int count);
void sym_add_change_count(int count);
-void conf_set_all_new_symbols(enum conf_def_mode mode);
+bool conf_set_all_new_symbols(enum conf_def_mode mode);
void set_all_choice_values(struct symbol *csym);
struct conf_printer {
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
is specified.
For example, given those two files (Thomas' test-case):
---8<--- Config.test.in
config OPTIONA
bool "Option A"
choice
prompt "This is a choice"
config CHOICE_OPTIONA
bool "Choice Option A"
config CHOICE_OPTIONB
bool "Choice Option B"
endchoice
config OPTIONB
bool "Option B"
---8<--- Config.test.in
---8<--- config.defaults
CONFIG_OPTIONA=y
---8<--- config.defaults
And running:
./scripts/kconfig/conf --randconfig Config.test.in
does properly randomise the two choice symbols (and the two booleans).
However, running:
KCONFIG_ALLCONFIG=config.defaults \
./scripts/kconfig/conf --randconfig Config.test.in
does *not* reandomise the two choice entries, and only CHOICE_OPTIONA
will ever be selected. (OPTIONA will always be set (expected), and
OPTIONB will be be properly randomised (expected).)
This patch defers setting that a choice has a value until a symbol for
that choice is indeed set, so that choices are properly randomised when
KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
Reported-by: Thomas Petazzoni <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Stephen Rothwell <[email protected]>
---
Changes v3 -> v4
- fix previous issue where some choices would not be set, which would
cause silentoldconfig to ask for them and was then breaking this
workflow (as reported by Arnd and Sedat):
KCONFIG_ALLCONFIG=foo.defconfig make randconfig
make silentoldconfig </dev/nullo
which I have tested (3h28min!) with:
touch defconfig
for(( i=0; i<10000; i++ )); do
KCONFIG_ALLCONFIG=$(pwd)/defconfig make randconfig >/dev/null 2>&1
make silentoldconfig </dev/null >/dev/null 2>&1 || break
done
which did not break at all.
- change done in v3 (below) is already fixed by a previous patch
Changes v2 -> v3
- ensure only one symbol is set in a choice
Changes v1 -> v2:
- further postpone setting that a choice has a value until
one is indeed set
- do not print symbols that are part of an invisible choice
---
scripts/kconfig/confdata.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c55c227..3e39208 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -288,8 +288,6 @@ load:
for_all_symbols(i, sym) {
sym->flags |= SYMBOL_CHANGED;
sym->flags &= ~(def_flags|SYMBOL_VALID);
- if (sym_is_choice(sym))
- sym->flags |= def_flags;
switch (sym->type) {
case S_INT:
case S_HEX:
@@ -379,13 +377,13 @@ setsym:
case mod:
if (cs->def[def].tri == yes) {
conf_warning("%s creates inconsistent choice state", sym->name);
- cs->flags &= ~def_flags;
}
break;
case yes:
if (cs->def[def].tri != no)
conf_warning("override: %s changes choice state", sym->name);
cs->def[def].val = sym;
+ cs->flags |= def_flags;
break;
}
cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
@@ -791,6 +789,8 @@ int conf_write(const char *name)
sym_calc_value(sym);
if (!(sym->flags & SYMBOL_WRITE))
goto next;
+ if (sym_is_choice_value(sym) && !menu_is_visible(menu->parent))
+ goto next;
sym->flags &= ~SYMBOL_WRITE;
conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
When searching for symbols, return the symbols sorted by relevance.
Sorting is done as thus:
- first, symbols that match exactly
- then, alphabetical sort
Since the search can be a regexp, it is possible that more than one symbol
matches exactly. In this case, we can't decide which to sort first, so we
fallback to alphabeticall sort.
Explain this (new!) sorting heuristic in the documentation.
Reported-by: Jean Delvare <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
Cc: Jean Delvare <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Roland Eggner <[email protected]>
Cc: Wang YanQing <[email protected]>
--
Changes v1->v2:
- drop the previous, complex heuristic in favour of a simpler heuristic
that is both easier to understand, *and* to maintain (Jean)
- explain sorting heuristic in the doc (Jean)
---
Documentation/kbuild/kconfig.txt | 13 +++++++
scripts/kconfig/symbol.c | 78 +++++++++++++++++++++++++++++++++++-----
2 files changed, 82 insertions(+), 9 deletions(-)
diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
index 3f429ed..e9f9e76 100644
--- a/Documentation/kbuild/kconfig.txt
+++ b/Documentation/kbuild/kconfig.txt
@@ -174,6 +174,19 @@ 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).
+ For example: ^ATH.K matches:
+ ATH5K ATH9K ATH5K_AHB ATH5K_DEBUG [...] ATH6KL ATH6KL_DEBUG
+ [...] ATH9K_AHB ATH9K_BTCOEX_SUPPORT ATH9K_COMMON [...]
+ of which only ATH5K and ATH9K match exactly and so are sorted
+ first (and in alphabetical order), then come all other symbols,
+ sorted in alphabetical order.
+
______________________________________________________________________
User interface options for 'menuconfig'
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index ab8f4c8..387d554 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -954,38 +954,98 @@ const char *sym_escape_string_value(const char *in)
return res;
}
+struct sym_match {
+ struct symbol *sym;
+ off_t so, eo;
+};
+
+/* Compare matched symbols as thus:
+ * - first, symbols that match exactly
+ * - then, alphabetical sort
+ */
+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;
+
+ /* Exact match:
+ * - if matched length on symbol s1 is the length of that symbol,
+ * then this symbol should come first;
+ * - if matched length on symbol s2 is the length of that symbol,
+ * then this symbol should come first.
+ * Note: since the search can be a regexp, both symbols may match
+ * 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))
+ return -1;
+ if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
+ return 1;
+
+ /* As a fallback, sort symbols alphabetically */
+ return strcmp(s1->sym->name, s2->sym->name);
+}
+
struct symbol **sym_re_search(const char *pattern)
{
struct symbol *sym, **sym_arr = NULL;
+ struct sym_match **sym_match_arr = NULL;
int i, cnt, size;
regex_t re;
+ regmatch_t match[1];
cnt = size = 0;
/* Skip if empty */
if (strlen(pattern) == 0)
return NULL;
- if (regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB|REG_ICASE))
+ if (regcomp(&re, pattern, REG_EXTENDED|REG_ICASE))
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, 0, NULL, 0))
+ if (regexec(&re, sym->name, 1, match, 0))
continue;
if (cnt + 1 >= size) {
- void *tmp = sym_arr;
+ void *tmp;
size += 16;
- sym_arr = realloc(sym_arr, size * sizeof(struct symbol *));
- if (!sym_arr) {
- free(tmp);
- return NULL;
+ 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);
- sym_arr[cnt++] = 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
+ * 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;
}
- if (sym_arr)
+ if (sym_match_arr) {
+ 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[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);
+ }
regfree(&re);
return sym_arr;
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
Reported-by: Jean Delvare <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
Cc: Jean Delvare <[email protected]>
Cc: Michal Marek <[email protected]>
---
scripts/kconfig/mconf.c | 2 +-
scripts/kconfig/nconf.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 6ee4aae..18d3dc9 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -401,7 +401,7 @@ static void search_conf(void)
struct subtitle_part stpart;
title = str_new();
- str_printf( &title, _("Enter %s (sub)string to search for "
+ str_printf( &title, _("Enter %s (sub)string or regexp to search for "
"(with or without \"%s\")"), CONFIG_, CONFIG_);
again:
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index 0183153..7975d8d 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -695,7 +695,7 @@ static void search_conf(void)
int dres;
title = str_new();
- str_printf( &title, _("Enter %s (sub)string to search for "
+ str_printf( &title, _("Enter %s (sub)string or regexp to search for "
"(with or without \"%s\")"), CONFIG_, CONFIG_);
again:
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
... so the user has a chance to reproduce a test-case.
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
scripts/kconfig/conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 94521c7..38616c1 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -532,6 +532,7 @@ int main(int ac, char **av)
seed = tmp;
}
}
+ fprintf( stderr, "KCONFIG_SEED=0x%X\n", seed );
srand(seed);
break;
}
--
1.8.1.2
From: Clement Chauplannaz <[email protected]>
The script `config' prints its name in usage() function. It is currently
hard-coded to value `config'. However, the script may be reused under
a different name in contexts other than the Linux Kernel.
Replace the hard-coded value `config' by the name of the script at runtime.
Signed-off-by: Clement Chauplannaz <[email protected]>
Acked-by: Andi Kleen <[email protected]>
Signed-off-by: Yann E. MORIN <[email protected]>
---
scripts/config | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/config b/scripts/config
index bb4d3de..6b3272e 100755
--- a/scripts/config
+++ b/scripts/config
@@ -1,6 +1,8 @@
#!/bin/bash
# Manipulate options in a .config file from the command line
+myname=${0##*/}
+
# If no prefix forced, use the default CONFIG_
CONFIG_="${CONFIG_-CONFIG_}"
@@ -8,7 +10,7 @@ usage() {
cat >&2 <<EOL
Manipulate options in a .config file from the command line.
Usage:
-config options command ...
+$myname options command ...
commands:
--enable|-e option Enable option
--disable|-d option Disable option
@@ -33,14 +35,14 @@ options:
--file config-file .config file to change (default .config)
--keep-case|-k Keep next symbols' case (dont' upper-case it)
-config doesn't check the validity of the .config file. This is done at next
+$myname doesn't check the validity of the .config file. This is done at next
make time.
-By default, config will upper-case the given symbol. Use --keep-case to keep
+By default, $myname will upper-case the given symbol. Use --keep-case to keep
the case of all following symbols unchanged.
-config uses 'CONFIG_' as the default symbol prefix. Set the environment
-variable CONFIG_ to the prefix to use. Eg.: CONFIG_="FOO_" config ...
+$myname uses 'CONFIG_' as the default symbol prefix. Set the environment
+variable CONFIG_ to the prefix to use. Eg.: CONFIG_="FOO_" $myname ...
EOL
exit 1
}
--
1.8.1.2
From: "Yann E. MORIN" <[email protected]>
Currently, randconfig may set more than one symbol in a given choice.
Given this config file:
config A
bool "A"
if A
choice
bool "B/C/D"
config B
bool "B"
config C
bool "C"
config D
bool "D"
endchoice
endif # A
Then randconfig generates such .config files (case where A is not set is not
shown below for brevity), and where only the right-most .config is valid:
CONFIG_A=y CONFIG_A=y CONFIG_A=y
CONFIG_B=y CONFIG_B=y CONFIG_B=y
CONFIG_C=y # CONFIG_C is not set # CONFIG_C is not set
# CONFIG_D is not set CONFIG_D=y # CONFIG_D is not set
That is, in a randomised choice, the first symbol is always selected,
and at most one other symbol may be selected.
This is due to symbol randomised in a choice not being properly flagged
as having a value.
Fix that by flagging those symbols adequately: have a user-defined value,
and be not valid (to force recalculation of the symbol).
Note: if the choice is not conditional, then the randomisation is properly
done.
Reported-by: Matthieu CASTET <[email protected]>
Signed-off-by: Matthieu CASTET <[email protected]>
[[email protected]: independently re-done the same patch as Matthieu,
as pointed out by Sedat]
Cc: Arnaud Lacombe <[email protected]>
Cc: Sedat Dilek <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
scripts/kconfig/confdata.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 35e0f16..d36bc1f 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1077,6 +1077,9 @@ static void randomize_choice_values(struct symbol *csym)
else {
sym->def[S_DEF_USER].tri = no;
}
+ sym->flags |= SYMBOL_DEF_USER;
+ /* clear VALID to get value calculated */
+ sym->flags &= ~SYMBOL_VALID;
}
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
--
1.8.1.2
From: Dirk Gouders <[email protected]>
Submenus are sometimes empty and it would be nice if there is
something that notifies us that we should not expect any content
_before_ we enter a submenu.
A new function menu_is_empty() was introduced and empty menus and
menuconfigs are now marked by "----" as opposed to non-empty ones that
are marked by "--->".
This scheme was suggested by "Yann E. MORIN" <[email protected]>.
Signed-off-by: Dirk Gouders <[email protected]>
Tested-by: "Yann E. MORIN" <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
scripts/kconfig/lkc_proto.h | 1 +
scripts/kconfig/mconf.c | 11 ++++++-----
scripts/kconfig/menu.c | 16 ++++++++++++++++
scripts/kconfig/nconf.c | 16 ++++++++--------
4 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index ef1a738..ecdb965 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -14,6 +14,7 @@ P(conf_set_message_callback, void,(void (*fn)(const char *fmt, va_list ap)));
/* menu.c */
P(rootmenu,struct menu,);
+P(menu_is_empty, bool, (struct menu *menu));
P(menu_is_visible, bool, (struct menu *menu));
P(menu_has_prompt, bool, (struct menu *menu));
P(menu_get_prompt,const char *,(struct menu *menu));
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index cb8cf4a..6ee4aae 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -48,7 +48,7 @@ static const char mconf_readme[] = N_(
"----------\n"
"o Use the Up/Down arrow keys (cursor keys) to highlight the item\n"
" you wish to change or submenu wish to select and press <Enter>.\n"
-" Submenus are designated by \"--->\".\n"
+" Submenus are designated by \"--->\", empty ones by \"----\".\n"
"\n"
" Shortcut: Press the option's highlighted letter (hotkey).\n"
" Pressing a hotkey more than once will sequence\n"
@@ -176,7 +176,7 @@ static const char mconf_readme[] = N_(
"\n"),
menu_instructions[] = N_(
"Arrow keys navigate the menu. "
- "<Enter> selects submenus --->. "
+ "<Enter> selects submenus ---> (or empty submenus ----). "
"Highlighted letters are hotkeys. "
"Pressing <Y> includes, <N> excludes, <M> modularizes features. "
"Press <Esc><Esc> to exit, <?> for Help, </> for Search. "
@@ -498,8 +498,9 @@ static void build_conf(struct menu *menu)
menu->data ? "-->" : "++>",
indent + 1, ' ', prompt);
} else
- item_make(" %*c%s --->", indent + 1, ' ', prompt);
-
+ item_make(" %*c%s %s",
+ indent + 1, ' ', prompt,
+ menu_is_empty(menu) ? "----" : "--->");
item_set_tag('m');
item_set_data(menu);
if (single_menu_mode && menu->data)
@@ -630,7 +631,7 @@ static void build_conf(struct menu *menu)
(sym_has_value(sym) || !sym_is_changable(sym)) ?
"" : _(" (NEW)"));
if (menu->prompt->type == P_MENU) {
- item_add_str(" --->");
+ item_add_str(" %s", menu_is_empty(menu) ? "----" : "--->");
return;
}
}
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index b5c7d90..6d11c8f 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -430,6 +430,22 @@ bool menu_has_prompt(struct menu *menu)
return true;
}
+/*
+ * Determine if a menu is empty.
+ * A menu is considered empty if it contains no or only
+ * invisible entries.
+ */
+bool menu_is_empty(struct menu *menu)
+{
+ struct menu *child;
+
+ for (child = menu->list; child; child = child->next) {
+ if (menu_is_visible(child))
+ return(false);
+ }
+ return(true);
+}
+
bool menu_is_visible(struct menu *menu)
{
struct menu *child;
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index aac85d3..0183153 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -45,8 +45,8 @@ static const char nconf_global_help[] = N_(
"<n> to remove it. You may press the <Space> key to cycle through the\n"
"available options.\n"
"\n"
-"A trailing \"--->\" designates a submenu.\n"
-"\n"
+"A trailing \"--->\" designates a submenu, a trailing \"----\" an\n"
+"empty submenu.\n"
"\n"
"Menu navigation keys\n"
"----------------------------------------------------------------------\n"
@@ -131,7 +131,7 @@ static const char nconf_global_help[] = N_(
"\n"),
menu_no_f_instructions[] = N_(
"Legend: [*] built-in [ ] excluded <M> module < > module capable.\n"
-"Submenus are designated by a trailing \"--->\".\n"
+"Submenus are designated by a trailing \"--->\", empty ones by \"----\".\n"
"\n"
"Use the following keys to navigate the menus:\n"
"Move up or down with <Up> and <Down>.\n"
@@ -148,7 +148,7 @@ menu_no_f_instructions[] = N_(
"For help related to the current menu entry press <?> or <h>.\n"),
menu_instructions[] = N_(
"Legend: [*] built-in [ ] excluded <M> module < > module capable.\n"
-"Submenus are designated by a trailing \"--->\".\n"
+"Submenus are designated by a trailing \"--->\", empty ones by \"----\".\n"
"\n"
"Use the following keys to navigate the menus:\n"
"Move up or down with <Up> or <Down>.\n"
@@ -760,9 +760,9 @@ static void build_conf(struct menu *menu)
indent + 1, ' ', prompt);
} else
item_make(menu, 'm',
- " %*c%s --->",
- indent + 1,
- ' ', prompt);
+ " %*c%s %s",
+ indent + 1, ' ', prompt,
+ menu_is_empty(menu) ? "----" : "--->");
if (single_menu_mode && menu->data)
goto conf_childs;
@@ -904,7 +904,7 @@ static void build_conf(struct menu *menu)
(sym_has_value(sym) || !sym_is_changable(sym)) ?
"" : _(" (NEW)"));
if (menu->prompt && menu->prompt->type == P_MENU) {
- item_add_str(" --->");
+ item_add_str(" %s", menu_is_empty(menu) ? "----" : "--->");
return;
}
}
--
1.8.1.2
From: Dirk Gouders <[email protected]>
According to the documentation [1], LINES and COLS are initialized by
initscr(); it does not say anything about the behavior when windows are
resized.
Do not rely on the current implementation of ncurses that updates
these variables on resize, but use the propper function calls to get
window dimensions.
init_dialog() could make use of the variables, but for the sake of
consistency we do not change it's current use of the macro getmaxyx().
[1] ncurses(3X)
Signed-off-by: Dirk Gouders <[email protected]>
Tested-by: "Yann E. MORIN" <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
Signed-off-by: Yann E. MORIN <[email protected]>
---
scripts/kconfig/lxdialog/checklist.c | 4 ++--
scripts/kconfig/lxdialog/inputbox.c | 4 ++--
scripts/kconfig/lxdialog/menubox.c | 4 ++--
scripts/kconfig/lxdialog/textbox.c | 4 ++--
scripts/kconfig/lxdialog/util.c | 13 +++++++++----
scripts/kconfig/lxdialog/yesno.c | 4 ++--
6 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/scripts/kconfig/lxdialog/checklist.c b/scripts/kconfig/lxdialog/checklist.c
index 3034057..3b15c08 100644
--- a/scripts/kconfig/lxdialog/checklist.c
+++ b/scripts/kconfig/lxdialog/checklist.c
@@ -140,8 +140,8 @@ do_resize:
max_choice = MIN(list_height, item_count());
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/inputbox.c b/scripts/kconfig/lxdialog/inputbox.c
index 7b01add..447a582 100644
--- a/scripts/kconfig/lxdialog/inputbox.c
+++ b/scripts/kconfig/lxdialog/inputbox.c
@@ -62,8 +62,8 @@ do_resize:
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/menubox.c b/scripts/kconfig/lxdialog/menubox.c
index 00d2841..92b89a6 100644
--- a/scripts/kconfig/lxdialog/menubox.c
+++ b/scripts/kconfig/lxdialog/menubox.c
@@ -203,8 +203,8 @@ do_resize:
max_choice = MIN(menu_height, item_count());
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
index 907cdcb..1773319 100644
--- a/scripts/kconfig/lxdialog/textbox.c
+++ b/scripts/kconfig/lxdialog/textbox.c
@@ -98,8 +98,8 @@ do_resize:
width = 0;
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index 0fa567e..58a8289 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -254,7 +254,12 @@ void attr_clear(WINDOW * win, int height, int width, chtype attr)
void dialog_clear(void)
{
- attr_clear(stdscr, LINES, COLS, dlg.screen.atr);
+ int lines, columns;
+
+ lines = getmaxy(stdscr);
+ columns = getmaxx(stdscr);
+
+ attr_clear(stdscr, lines, columns, dlg.screen.atr);
/* Display background title if it exists ... - SLH */
if (dlg.backtitle != NULL) {
int i, len = 0, skip = 0;
@@ -269,10 +274,10 @@ void dialog_clear(void)
}
wmove(stdscr, 1, 1);
- if (len > COLS - 2) {
+ if (len > columns - 2) {
const char *ellipsis = "[...] ";
waddstr(stdscr, ellipsis);
- skip = len - (COLS - 2 - strlen(ellipsis));
+ skip = len - (columns - 2 - strlen(ellipsis));
}
for (pos = dlg.subtitles; pos != NULL; pos = pos->next) {
@@ -298,7 +303,7 @@ void dialog_clear(void)
skip--;
}
- for (i = len + 1; i < COLS - 1; i++)
+ for (i = len + 1; i < columns - 1; i++)
waddch(stdscr, ACS_HLINE);
}
wnoutrefresh(stdscr);
diff --git a/scripts/kconfig/lxdialog/yesno.c b/scripts/kconfig/lxdialog/yesno.c
index abb0c39..676fb2f 100644
--- a/scripts/kconfig/lxdialog/yesno.c
+++ b/scripts/kconfig/lxdialog/yesno.c
@@ -51,8 +51,8 @@ do_resize:
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
--
1.8.1.2
From: Dirk Gouders <[email protected]>
According to the documentation [1], LINES and COLS are initialized by
initscr(); it does not say anything about the behavior when windows are
resized.
Do not rely on the current implementation of ncurses that updates
these variables on resize, but use the propper function calls or macros
to get window dimensions.
The use of the variables in main() was OK, but for the sake of
consistency it was modified to use the macro getmaxyx().
[1] ncurses(3X)
Signed-off-by: Dirk Gouders <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
[[email protected]: declare 'lines' and 'columns' on a single line]
Signed-off-by: Yann E. MORIN <[email protected]>
---
scripts/kconfig/nconf.c | 21 ++++++++++++++-------
scripts/kconfig/nconf.gui.c | 20 +++++++++++---------
2 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index dbf31ed..aac85d3 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -365,15 +365,16 @@ static void print_function_line(void)
int i;
int offset = 1;
const int skip = 1;
+ int lines = getmaxy(stdscr);
for (i = 0; i < function_keys_num; i++) {
(void) wattrset(main_window, attributes[FUNCTION_HIGHLIGHT]);
- mvwprintw(main_window, LINES-3, offset,
+ mvwprintw(main_window, lines-3, offset,
"%s",
function_keys[i].key_str);
(void) wattrset(main_window, attributes[FUNCTION_TEXT]);
offset += strlen(function_keys[i].key_str);
- mvwprintw(main_window, LINES-3,
+ mvwprintw(main_window, lines-3,
offset, "%s",
function_keys[i].func);
offset += strlen(function_keys[i].func) + skip;
@@ -954,7 +955,7 @@ static void show_menu(const char *prompt, const char *instructions,
clear();
(void) wattrset(main_window, attributes[NORMAL]);
- print_in_middle(stdscr, 1, 0, COLS,
+ print_in_middle(stdscr, 1, 0, getmaxx(stdscr),
menu_backtitle,
attributes[MAIN_HEADING]);
@@ -1455,14 +1456,18 @@ static void conf_save(void)
void setup_windows(void)
{
+ int lines, columns;
+
+ getmaxyx(stdscr, lines, columns);
+
if (main_window != NULL)
delwin(main_window);
/* set up the menu and menu window */
- main_window = newwin(LINES-2, COLS-2, 2, 1);
+ main_window = newwin(lines-2, columns-2, 2, 1);
keypad(main_window, TRUE);
- mwin_max_lines = LINES-7;
- mwin_max_cols = COLS-6;
+ mwin_max_lines = lines-7;
+ mwin_max_cols = columns-6;
/* panels order is from bottom to top */
new_panel(main_window);
@@ -1470,6 +1475,7 @@ void setup_windows(void)
int main(int ac, char **av)
{
+ int lines, columns;
char *mode;
setlocale(LC_ALL, "");
@@ -1495,7 +1501,8 @@ int main(int ac, char **av)
keypad(stdscr, TRUE);
curs_set(0);
- if (COLS < 75 || LINES < 20) {
+ getmaxyx(stdscr, lines, columns);
+ if (columns < 75 || lines < 20) {
endwin();
printf("Your terminal should have at "
"least 20 lines and 75 columns\n");
diff --git a/scripts/kconfig/nconf.gui.c b/scripts/kconfig/nconf.gui.c
index 9f8c44e..8275f0e5 100644
--- a/scripts/kconfig/nconf.gui.c
+++ b/scripts/kconfig/nconf.gui.c
@@ -276,8 +276,8 @@ int btn_dialog(WINDOW *main_window, const char *msg, int btn_num, ...)
total_width = max(msg_width, btns_width);
/* place dialog in middle of screen */
- y = (LINES-(msg_lines+4))/2;
- x = (COLS-(total_width+4))/2;
+ y = (getmaxy(stdscr)-(msg_lines+4))/2;
+ x = (getmaxx(stdscr)-(total_width+4))/2;
/* create the windows */
@@ -387,8 +387,8 @@ int dialog_inputbox(WINDOW *main_window,
prompt_width = max(prompt_width, strlen(title));
/* place dialog in middle of screen */
- y = (LINES-(prompt_lines+4))/2;
- x = (COLS-(prompt_width+4))/2;
+ y = (getmaxy(stdscr)-(prompt_lines+4))/2;
+ x = (getmaxx(stdscr)-(prompt_width+4))/2;
strncpy(result, init, *result_len);
@@ -545,7 +545,7 @@ void show_scroll_win(WINDOW *main_window,
{
int res;
int total_lines = get_line_no(text);
- int x, y;
+ int x, y, lines, columns;
int start_x = 0, start_y = 0;
int text_lines = 0, text_cols = 0;
int total_cols = 0;
@@ -556,6 +556,8 @@ void show_scroll_win(WINDOW *main_window,
WINDOW *pad;
PANEL *panel;
+ getmaxyx(stdscr, lines, columns);
+
/* find the widest line of msg: */
total_lines = get_line_no(text);
for (i = 0; i < total_lines; i++) {
@@ -569,14 +571,14 @@ void show_scroll_win(WINDOW *main_window,
(void) wattrset(pad, attributes[SCROLLWIN_TEXT]);
fill_window(pad, text);
- win_lines = min(total_lines+4, LINES-2);
- win_cols = min(total_cols+2, COLS-2);
+ win_lines = min(total_lines+4, lines-2);
+ win_cols = min(total_cols+2, columns-2);
text_lines = max(win_lines-4, 0);
text_cols = max(win_cols-2, 0);
/* place window in middle of screen */
- y = (LINES-win_lines)/2;
- x = (COLS-win_cols)/2;
+ y = (lines-win_lines)/2;
+ x = (columns-win_cols)/2;
win = newwin(win_lines, win_cols, y, x);
keypad(win, TRUE);
--
1.8.1.2
From: Dirk Gouders <[email protected]>
When exiting menuconfig with unsaved changes, a dialog like
the following is shown:
Do you wish to save your new configuration ? <ESC><ESC>
to continue.
The author of the dialog text specified a newline after the '?',
and probably expected it to be processed, so let print_autowrap()
handle newlines propperly.
Also, reword that dialog's second phrase with a real sentence.
Signed-off-by: Dirk Gouders <[email protected]>
Tested-by: "Yann E. MORIN" <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
[[email protected]: very slightly tweak the commit message]
Signed-off-by: Yann E. MORIN <[email protected]>
---
scripts/kconfig/lxdialog/util.c | 31 +++++++++++++++++--------------
scripts/kconfig/mconf.c | 4 ++--
2 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index 78fe4e9..0fa567e 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -371,27 +371,19 @@ void print_title(WINDOW *dialog, const char *title, int width)
/*
* Print a string of text in a window, automatically wrap around to the
* next line if the string is too long to fit on one line. Newline
- * characters '\n' are replaced by spaces. We start on a new line
+ * characters '\n' are propperly processed. We start on a new line
* if there is no room for at least 4 nonblanks following a double-space.
*/
void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
{
int newl, cur_x, cur_y;
- int i, prompt_len, room, wlen;
- char tempstr[MAX_LEN + 1], *word, *sp, *sp2;
+ int prompt_len, room, wlen;
+ char tempstr[MAX_LEN + 1], *word, *sp, *sp2, *newline_separator = 0;
strcpy(tempstr, prompt);
prompt_len = strlen(tempstr);
- /*
- * Remove newlines
- */
- for (i = 0; i < prompt_len; i++) {
- if (tempstr[i] == '\n')
- tempstr[i] = ' ';
- }
-
if (prompt_len <= width - x * 2) { /* If prompt is short */
wmove(win, y, (width - prompt_len) / 2);
waddstr(win, tempstr);
@@ -401,7 +393,10 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
newl = 1;
word = tempstr;
while (word && *word) {
- sp = strchr(word, ' ');
+ sp = strpbrk(word, "\n ");
+ if (sp && *sp == '\n')
+ newline_separator = sp;
+
if (sp)
*sp++ = 0;
@@ -413,7 +408,7 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
if (wlen > room ||
(newl && wlen < 4 && sp
&& wlen + 1 + strlen(sp) > room
- && (!(sp2 = strchr(sp, ' '))
+ && (!(sp2 = strpbrk(sp, "\n "))
|| wlen + 1 + (sp2 - sp) > room))) {
cur_y++;
cur_x = x;
@@ -421,7 +416,15 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
wmove(win, cur_y, cur_x);
waddstr(win, word);
getyx(win, cur_y, cur_x);
- cur_x++;
+
+ /* Move to the next line if the word separator was a newline */
+ if (newline_separator) {
+ cur_y++;
+ cur_x = x;
+ newline_separator = 0;
+ } else
+ cur_x++;
+
if (sp && *sp == ' ') {
cur_x++; /* double space */
while (*++sp == ' ') ;
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 2396c5b..cb8cf4a 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -958,8 +958,8 @@ static int handle_exit(void)
dialog_clear();
if (conf_get_changed())
res = dialog_yesno(NULL,
- _("Do you wish to save your new configuration ?\n"
- "<ESC><ESC> to continue."),
+ _("Do you wish to save your new configuration?\n"
+ "(Press <ESC><ESC> to continue kernel configuration.)"),
6, 60);
else
res = -1;
--
1.8.1.2
From: Sedat Dilek <[email protected]>
Commit c8dc68ad0fbd ("kconfig/lxdialog: support resize") added support
for resizing, but forgot to collect all hardcoded values at one single
place.
Also add a definition for the check for a minimum screen/window size
of 80x19.
[ ChangeLog v3:
* Rename MENU_{HEIGTH,WIDTH}_MIN -> MENUBOX_{HEIGTH,WIDTH}_MIN
ChangeLog v2:
* Rename WIN_{HEIGTH,WIDTH}_MIN -> WINDOW_{HEIGTH,WIDTH}_MIN
* Mention the check for a minimum screen/window size in the changelog
* Add a comment above the block of new definitions ]
Signed-off-by: Sedat Dilek <[email protected]>
Acked-by: Wang YanQing <[email protected]>
Tested-by: "Yann E. MORIN" <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
scripts/kconfig/lxdialog/checklist.c | 4 ++--
scripts/kconfig/lxdialog/dialog.h | 14 ++++++++++++++
scripts/kconfig/lxdialog/inputbox.c | 4 ++--
scripts/kconfig/lxdialog/menubox.c | 2 +-
scripts/kconfig/lxdialog/textbox.c | 2 +-
scripts/kconfig/lxdialog/util.c | 2 +-
scripts/kconfig/lxdialog/yesno.c | 4 ++--
7 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/scripts/kconfig/lxdialog/checklist.c b/scripts/kconfig/lxdialog/checklist.c
index a2eb80f..3034057 100644
--- a/scripts/kconfig/lxdialog/checklist.c
+++ b/scripts/kconfig/lxdialog/checklist.c
@@ -132,9 +132,9 @@ int dialog_checklist(const char *title, const char *prompt, int height,
}
do_resize:
- if (getmaxy(stdscr) < (height + 6))
+ if (getmaxy(stdscr) < (height + CHECKLIST_HEIGTH_MIN))
return -ERRDISPLAYTOOSMALL;
- if (getmaxx(stdscr) < (width + 6))
+ if (getmaxx(stdscr) < (width + CHECKLIST_WIDTH_MIN))
return -ERRDISPLAYTOOSMALL;
max_choice = MIN(list_height, item_count());
diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
index 10993370..b4343d3 100644
--- a/scripts/kconfig/lxdialog/dialog.h
+++ b/scripts/kconfig/lxdialog/dialog.h
@@ -200,6 +200,20 @@ int item_is_tag(char tag);
int on_key_esc(WINDOW *win);
int on_key_resize(void);
+/* minimum (re)size values */
+#define CHECKLIST_HEIGTH_MIN 6 /* For dialog_checklist() */
+#define CHECKLIST_WIDTH_MIN 6
+#define INPUTBOX_HEIGTH_MIN 2 /* For dialog_inputbox() */
+#define INPUTBOX_WIDTH_MIN 2
+#define MENUBOX_HEIGTH_MIN 15 /* For dialog_menu() */
+#define MENUBOX_WIDTH_MIN 65
+#define TEXTBOX_HEIGTH_MIN 8 /* For dialog_textbox() */
+#define TEXTBOX_WIDTH_MIN 8
+#define YESNO_HEIGTH_MIN 4 /* For dialog_yesno() */
+#define YESNO_WIDTH_MIN 4
+#define WINDOW_HEIGTH_MIN 19 /* For init_dialog() */
+#define WINDOW_WIDTH_MIN 80
+
int init_dialog(const char *backtitle);
void set_dialog_backtitle(const char *backtitle);
void set_dialog_subtitles(struct subtitle_list *subtitles);
diff --git a/scripts/kconfig/lxdialog/inputbox.c b/scripts/kconfig/lxdialog/inputbox.c
index 21404a0..7b01add 100644
--- a/scripts/kconfig/lxdialog/inputbox.c
+++ b/scripts/kconfig/lxdialog/inputbox.c
@@ -56,9 +56,9 @@ int dialog_inputbox(const char *title, const char *prompt, int height, int width
strcpy(instr, init);
do_resize:
- if (getmaxy(stdscr) <= (height - 2))
+ if (getmaxy(stdscr) <= (height - INPUTBOX_HEIGTH_MIN))
return -ERRDISPLAYTOOSMALL;
- if (getmaxx(stdscr) <= (width - 2))
+ if (getmaxx(stdscr) <= (width - INPUTBOX_WIDTH_MIN))
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
diff --git a/scripts/kconfig/lxdialog/menubox.c b/scripts/kconfig/lxdialog/menubox.c
index 48d382e..00d2841 100644
--- a/scripts/kconfig/lxdialog/menubox.c
+++ b/scripts/kconfig/lxdialog/menubox.c
@@ -193,7 +193,7 @@ int dialog_menu(const char *title, const char *prompt,
do_resize:
height = getmaxy(stdscr);
width = getmaxx(stdscr);
- if (height < 15 || width < 65)
+ if (height < MENUBOX_HEIGTH_MIN || width < MENUBOX_WIDTH_MIN)
return -ERRDISPLAYTOOSMALL;
height -= 4;
diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
index a48bb93..907cdcb 100644
--- a/scripts/kconfig/lxdialog/textbox.c
+++ b/scripts/kconfig/lxdialog/textbox.c
@@ -80,7 +80,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
do_resize:
getmaxyx(stdscr, height, width);
- if (height < 8 || width < 8)
+ if (height < TEXTBOX_HEIGTH_MIN || width < TEXTBOX_WIDTH_MIN)
return -ERRDISPLAYTOOSMALL;
if (initial_height != 0)
height = initial_height;
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index a0e97c2..78fe4e9 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -317,7 +317,7 @@ int init_dialog(const char *backtitle)
getyx(stdscr, saved_y, saved_x);
getmaxyx(stdscr, height, width);
- if (height < 19 || width < 80) {
+ if (height < WINDOW_HEIGTH_MIN || width < WINDOW_WIDTH_MIN) {
endwin();
return -ERRDISPLAYTOOSMALL;
}
diff --git a/scripts/kconfig/lxdialog/yesno.c b/scripts/kconfig/lxdialog/yesno.c
index 4e6e809..abb0c39 100644
--- a/scripts/kconfig/lxdialog/yesno.c
+++ b/scripts/kconfig/lxdialog/yesno.c
@@ -45,9 +45,9 @@ int dialog_yesno(const char *title, const char *prompt, int height, int width)
WINDOW *dialog;
do_resize:
- if (getmaxy(stdscr) < (height + 4))
+ if (getmaxy(stdscr) < (height + YESNO_HEIGTH_MIN))
return -ERRDISPLAYTOOSMALL;
- if (getmaxx(stdscr) < (width + 4))
+ if (getmaxx(stdscr) < (width + YESNO_WIDTH_MIN))
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
--
1.8.1.2
From: Sedat Dilek <[email protected]>
This is a cleanup which uses the proper (new) definitions and does
not change current behaviour.
Signed-off-by: Sedat Dilek <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
Tested-by: "Yann E. MORIN" <[email protected]>
Signed-off-by: "Yann E. MORIN" <[email protected]>
---
Yann had some more ideas on improvements:
"What would be nice is an improvement that scales the choice window to
the number of entries in the choice. If there are a lot of choice
entries, then the choice popup grows in height (but does not overflow
the screen of course). So, instead of seeing only 6 entries, we'd see
as much as possible in the current screen.
Ditto for the width: the popup adapts to the longest prompt (but does
not overflow the screen either, of course), so prompts are not
truncated."
NOTE: This patch requires [1].
[1] http://marc.info/?l=linux-kbuild&m=137128726917166&w=2
---
scripts/kconfig/mconf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 387dc8d..2396c5b 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -825,7 +825,9 @@ static void conf_choice(struct menu *menu)
dialog_clear();
res = dialog_checklist(prompt ? _(prompt) : _("Main Menu"),
_(radiolist_instructions),
- 15, 70, 6);
+ MENUBOX_HEIGTH_MIN,
+ MENUBOX_WIDTH_MIN,
+ CHECKLIST_HEIGTH_MIN);
selected = item_activate_selected();
switch (res) {
case 0:
--
1.8.1.2
From: Arve Hjønnevåg <[email protected]>
The defconfig and Kconfig combination below, which is based on 3.10-rc4
Kconfigs, resulted in several options getting set to "m" instead of "y".
defconfig.choice:
---8<---
CONFIG_MODULES=y
CONFIG_USB_ZERO=y
---8<---
Kconfig.choice:
---8<---
menuconfig MODULES
bool "Enable loadable module support"
config CONFIGFS_FS
tristate "Userspace-driven configuration filesystem"
config OCFS2_FS
tristate "OCFS2 file system support"
depends on CONFIGFS_FS
select CRC32
config USB_LIBCOMPOSITE
tristate
select CONFIGFS_FS
choice
tristate "USB Gadget Drivers"
default USB_ETH
config USB_ZERO
tristate "Gadget Zero (DEVELOPMENT)"
select USB_LIBCOMPOSITE
config USB_ETH
tristate "Ethernet Gadget (with CDC Ethernet support)"
select USB_LIBCOMPOSITE
endchoice
config CRC32
tristate "CRC32/CRC32c functions"
default y
choice
prompt "CRC32 implementation"
depends on CRC32
default CRC32_SLICEBY8
config CRC32_SLICEBY8
bool "Slice by 8 bytes"
endchoice
---8<---
$ scripts/kconfig/conf --defconfig=defconfig.choice Kconfig.choice
would result in:
.config:
---8<---
CONFIG_MODULES=y
CONFIG_CONFIGFS_FS=m
CONFIG_USB_LIBCOMPOSITE=m
CONFIG_USB_ZERO=m
CONFIG_CRC32=y
CONFIG_CRC32_SLICEBY8=y
---8<---
when the expected result would be:
.config:
---8<---
CONFIG_MODULES=y
CONFIG_CONFIGFS_FS=y
CONFIG_USB_LIBCOMPOSITE=y
CONFIG_USB_ZERO=y
CONFIG_CRC32=y
CONFIG_CRC32_SLICEBY8=y
---8<---
Signed-off-by: Arve Hjønnevåg <[email protected]>
[[email protected]: add the resulting .config to commit log,
remove unneeded USB_GADGET from the defconfig]
Tested-by: "Yann E. MORIN" <[email protected]>
Reviewed-by: "Yann E. MORIN" <[email protected]>
Signed-off-by: Yann E. MORIN <[email protected]>
---
scripts/kconfig/confdata.c | 14 ++++++++++----
scripts/kconfig/expr.h | 3 +++
scripts/kconfig/lkc.h | 1 +
scripts/kconfig/symbol.c | 11 +++++++++++
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 43eda40..35e0f16 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1083,7 +1083,7 @@ static void randomize_choice_values(struct symbol *csym)
csym->flags &= ~(SYMBOL_VALID);
}
-static void set_all_choice_values(struct symbol *csym)
+void set_all_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
@@ -1100,7 +1100,7 @@ static void set_all_choice_values(struct symbol *csym)
}
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
- csym->flags &= ~(SYMBOL_VALID);
+ csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
void conf_set_all_new_symbols(enum conf_def_mode mode)
@@ -1202,6 +1202,14 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
* selected in a choice block and we set it to yes,
* and the rest to no.
*/
+ if (mode != def_random) {
+ for_all_symbols(i, csym) {
+ if ((sym_is_choice(csym) && !sym_has_value(csym)) ||
+ sym_is_choice_value(csym))
+ csym->flags |= SYMBOL_NEED_SET_CHOICE_VALUES;
+ }
+ }
+
for_all_symbols(i, csym) {
if (sym_has_value(csym) || !sym_is_choice(csym))
continue;
@@ -1209,7 +1217,5 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
randomize_choice_values(csym);
- else
- set_all_choice_values(csym);
}
}
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index cdd4860..df198a5 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -106,6 +106,9 @@ struct symbol {
#define SYMBOL_DEF3 0x40000 /* symbol.def[S_DEF_3] is valid */
#define SYMBOL_DEF4 0x80000 /* symbol.def[S_DEF_4] is valid */
+/* choice values need to be set before calculating this symbol value */
+#define SYMBOL_NEED_SET_CHOICE_VALUES 0x100000
+
#define SYMBOL_MAXLENGTH 256
#define SYMBOL_HASHSIZE 9973
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index f8aee5f..0c8d419 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -87,6 +87,7 @@ char *conf_get_default_confname(void);
void sym_set_change_count(int count);
void sym_add_change_count(int count);
void conf_set_all_new_symbols(enum conf_def_mode mode);
+void set_all_choice_values(struct symbol *csym);
struct conf_printer {
void (*print_symbol)(FILE *, struct symbol *, const char *, void *);
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index ecc5aa5..ab8f4c8 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -300,6 +300,14 @@ void sym_calc_value(struct symbol *sym)
if (sym->flags & SYMBOL_VALID)
return;
+
+ if (sym_is_choice_value(sym) &&
+ sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES) {
+ sym->flags &= ~SYMBOL_NEED_SET_CHOICE_VALUES;
+ prop = sym_get_choice_prop(sym);
+ sym_calc_value(prop_get_symbol(prop));
+ }
+
sym->flags |= SYMBOL_VALID;
oldval = sym->curr;
@@ -425,6 +433,9 @@ void sym_calc_value(struct symbol *sym)
if (sym->flags & SYMBOL_AUTO)
sym->flags &= ~SYMBOL_WRITE;
+
+ if (sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES)
+ set_all_choice_values(sym);
}
void sym_clear_all_valid(void)
--
1.8.1.2
Dne 24.6.2013 20:11, Yann E. MORIN napsal(a):
> From: "Yann E. MORIN" <[email protected]>
>
> Hello Michal, All!
> │
> Michal, please pull these patches against kconfig that I have accumulated
> for 3.11.
>
> Note-whorthy this time:
> - fix values of tristates that are selected by boolean choices (Arve)
> - fix choice randomisation in presence of KCONFIG_ALLCONFIG (me)
> - fix choice randomisation selecting more than one value in
> a choice (but only if it is conditional) (me)
> - fix choice-in-a-choice randomisation not selecting any value
> for the inner-most choice (me)
>
> Also, some code-cleanups and eye-candy:
> - mconf and nconf code cleanups (Dirk, Sedat)
> - mconf and nconf eye-candy (Dirk, me)
> - scripts/config script-name in help text (Clément)
> - heuristic to sort found symbols by relevance (me)
> - more randconfig debugging help (me)
>
> Changes v1 -> v2:
> - simplify sorting heuristic (Jean)
> - make it clear in mconf/nconf that the search expression can
> be a regexp (Jean)
Pulled, thanks a lot!
Michal
On Mon, Jun 24, 2013 at 8:11 PM, Yann E. MORIN <[email protected]> wrote:
> From: "Yann E. MORIN" <[email protected]>
>
> Currently, randconfig may set more than one symbol in a given choice.
> Given this config file:
> config A
> bool "A"
> if A
> choice
> bool "B/C/D"
> config B
> bool "B"
> config C
> bool "C"
> config D
> bool "D"
> endchoice
> endif # A
>
> Then randconfig generates such .config files (case where A is not set is not
> shown below for brevity), and where only the right-most .config is valid:
> CONFIG_A=y CONFIG_A=y CONFIG_A=y
> CONFIG_B=y CONFIG_B=y CONFIG_B=y
> CONFIG_C=y # CONFIG_C is not set # CONFIG_C is not set
> # CONFIG_D is not set CONFIG_D=y # CONFIG_D is not set
>
> That is, in a randomised choice, the first symbol is always selected,
> and at most one other symbol may be selected.
>
> This is due to symbol randomised in a choice not being properly flagged
> as having a value.
>
> Fix that by flagging those symbols adequately: have a user-defined value,
> and be not valid (to force recalculation of the symbol).
>
> Note: if the choice is not conditional, then the randomisation is properly
> done.
>
> Reported-by: Matthieu CASTET <[email protected]>
> Signed-off-by: Matthieu CASTET <[email protected]>
> [[email protected]: independently re-done the same patch as Matthieu,
> as pointed out by Sedat]
> Cc: Arnaud Lacombe <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
[ CC Alexander ]
Can you or Marek (kbuild maintainer) please add also a Reported-by of
Alexander (see [1])?
Reported-by: Alexander Kriegisch <[email protected]>
It's a bit a pity that you discovered this issue by yourself independantly.
As said in PM, the Freetz router project has included it when playing
with Kconfig of Linux-v3.1-rc9.
Thanks for taking care.
- Sedat -
[1] http://www.spinics.net/lists/linux-kbuild/msg05702.html
[2] http://freetz.org/browser/trunk/tools/make/patches/340-fix_randconfig_choice.kconfig.patch
> ---
> scripts/kconfig/confdata.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 35e0f16..d36bc1f 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -1077,6 +1077,9 @@ static void randomize_choice_values(struct symbol *csym)
> else {
> sym->def[S_DEF_USER].tri = no;
> }
> + sym->flags |= SYMBOL_DEF_USER;
> + /* clear VALID to get value calculated */
> + sym->flags &= ~SYMBOL_VALID;
> }
> csym->flags |= SYMBOL_DEF_USER;
> /* clear VALID to get value calculated */
> --
> 1.8.1.2
>
Michal, All,
On Monday 24 June 2013 20:11:59 Yann E. MORIN wrote:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> This patch defers setting that a choice has a value until a symbol for
> that choice is indeed set, so that choices are properly randomised when
> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
I've just been informed by Fengguang that this patch breaks yet another
use-case of his, but this time KCONFIG_ALLCONFIG is not involved.
I'll investigate this issue when I'm back home tonight (ie. ~16:00 UTC).
Sorry for the breakage (again...). :-(
We really need a known, shared test-suite for kconfig... :-/
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. |
'------------------------------'-------'------------------'--------------------'
On Tue, Jun 25, 2013 at 10:01 AM, Yann E. MORIN <[email protected]> wrote:
> Michal, All,
>
> On Monday 24 June 2013 20:11:59 Yann E. MORIN wrote:
>> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
>> is specified.
> [--SNIP--]
>> This patch defers setting that a choice has a value until a symbol for
>> that choice is indeed set, so that choices are properly randomised when
>> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
>
> I've just been informed by Fengguang that this patch breaks yet another
> use-case of his, but this time KCONFIG_ALLCONFIG is not involved.
>
> I'll investigate this issue when I'm back home tonight (ie. ~16:00 UTC).
> Sorry for the breakage (again...). :-(
>
> We really need a known, shared test-suite for kconfig... :-/
>
[ CC Alexander ]
A-propos, "testing Kconfig" I recommended to take into account the
script of Alexander.
In PM we discussed on where to place such scripts and that Alexander's
patch needs some refreshing in your eyes.
Maybe, Alexander can/will help as he is the original author.
As said, this script helped in the Freetz router project to eliminate
dozens of (logically) wrong Kconfig settings.
IMHO there exist a lot of more, as there are "double/triple" checks in
the Freetz-buildsystem, but hey noone is perfect :-).
- Sedat -
[1] http://freetz.org/browser/trunk/tools/developer/create-kconfig-warnings
> 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. |
> '------------------------------'-------'------------------'--------------------'
Michal, All,
On 2013-06-24 20:11 +0200, Yann E. MORIN spake thusly:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> This patch defers setting that a choice has a value until a symbol for
> that choice is indeed set, so that choices are properly randomised when
> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
OK, so this second attempt also broke standard workflows. Fixing it is
more involved than expected. Let's get rid of it before too many people
complain, and before it hits mainline.
Michal, can you strip this from your tree (since it's the HEAD of your
kconfig branch), or do you want me to submit a revert patch?
Regards,
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. |
'------------------------------^-------^------------------^--------------------'
Hi Yann,
Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <[email protected]>
>
> When searching for symbols, return the symbols sorted by relevance.
>
> Sorting is done as thus:
> - first, symbols that match exactly
> - then, alphabetical sort
I like it, this is simple and efficient.
> Since the search can be a regexp, it is possible that more than one symbol
> matches exactly. In this case, we can't decide which to sort first, so we
> fallback to alphabeticall sort.
Typo: alphabetical.
> Explain this (new!) sorting heuristic in the documentation.
It's more of a rule than a heuristic.
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
> Cc: Jean Delvare <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Roland Eggner <[email protected]>
> Cc: Wang YanQing <[email protected]>
I tested it and it works fine, thanks!
Tested-by: Jean Delvare <[email protected]>
Now comes my late review. Overall I like the idea and the code but a few
things could be improved:
> --
> Changes v1->v2:
> - drop the previous, complex heuristic in favour of a simpler heuristic
> that is both easier to understand, *and* to maintain (Jean)
> - explain sorting heuristic in the doc (Jean)
> ---
> Documentation/kbuild/kconfig.txt | 13 +++++++
> scripts/kconfig/symbol.c | 78 +++++++++++++++++++++++++++++++++++-----
> 2 files changed, 82 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
> index 3f429ed..e9f9e76 100644
> --- a/Documentation/kbuild/kconfig.txt
> +++ b/Documentation/kbuild/kconfig.txt
> @@ -174,6 +174,19 @@ 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;
The use of singular seems to imply there can be only one, while there
could be several as you explained above.
> + - alphabetical order: when two symbols do not match exactly,
> + they are sorted in alphabetical order (in the user's current
> + locale).
I think it would be better explained that way:
- first, exact matches, sorted alphabetically (an exact match
is when the search matches the complete symbol name);
- then, other matches, sorted alphabetically.
This is more concise and easier to grasp, methinks. I don't think the
reference to the user's locale is needed, as there's no surprise here,
and it probably doesn't matter anyway for kernel symbols.
BTW I was wondering if it would add value to explicitly print group
header labels "Exact matches" and "Other matches" in the search results.
What do you think?
> + For example: ^ATH.K matches:
> + ATH5K ATH9K ATH5K_AHB ATH5K_DEBUG [...] ATH6KL ATH6KL_DEBUG
> + [...] ATH9K_AHB ATH9K_BTCOEX_SUPPORT ATH9K_COMMON [...]
> + of which only ATH5K and ATH9K match exactly and so are sorted
> + first (and in alphabetical order), then come all other symbols,
> + sorted in alphabetical order.
Very clear example :)
> ______________________________________________________________________
> User interface options for 'menuconfig'
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index ab8f4c8..387d554 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -954,38 +954,98 @@ const char *sym_escape_string_value(const char *in)
> return res;
> }
>
> +struct sym_match {
> + struct symbol *sym;
> + off_t so, eo;
> +};
> +
> +/* Compare matched symbols as thus:
> + * - first, symbols that match exactly
> + * - then, alphabetical sort
> + */
> +static int sym_rel_comp( const void *sym1, const void *sym2 )
Coding style says no space after/before parentheses.
> +{
> + struct sym_match *s1 = *(struct sym_match **)sym1;
> + struct sym_match *s2 = *(struct sym_match **)sym2;
You shouldn't need these casts.
> + int l1, l2;
> +
> + /* Exact match:
> + * - if matched length on symbol s1 is the length of that symbol,
> + * then this symbol should come first;
> + * - if matched length on symbol s2 is the length of that symbol,
> + * then this symbol should come first.
> + * Note: since the search can be a regexp, both symbols may match
> + * 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))
> + return -1;
> + if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
> + return 1;
Strlen is expensive so I would avoid calling it twice on the same symbol
name. You could store the result, together with the result of the
comparison, while you're at it. Something like:
int exact1, exact2;
exact1 = l1 == strlen(s1->sym->name);
exact2 = l2 == strlen(s2->sym->name);
if (exact1 && !exact2)
return -1;
if (!exact1 && exact2)
return 1;
You may even no longer need to introduce l1 and l2 as you would use them
only once.
It might be even faster to store the symbol length in struct sym_match,
but this would increase the structure size and consequently memory
consumption, and it is questionable if the speed gain is worth it.
Probably not.
> +
> + /* As a fallback, sort symbols alphabetically */
> + return strcmp(s1->sym->name, s2->sym->name);
> +}
> +
> struct symbol **sym_re_search(const char *pattern)
> {
> struct symbol *sym, **sym_arr = NULL;
> + struct sym_match **sym_match_arr = NULL;
> int i, cnt, size;
> regex_t re;
> + regmatch_t match[1];
>
> cnt = size = 0;
> /* Skip if empty */
> if (strlen(pattern) == 0)
> return NULL;
> - if (regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB|REG_ICASE))
> + if (regcomp(&re, pattern, REG_EXTENDED|REG_ICASE))
> 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, 0, NULL, 0))
> + if (regexec(&re, sym->name, 1, match, 0))
> continue;
> if (cnt + 1 >= size) {
I think the "+ 1" can be dropped because the new array is not
NULL-terminated.
> - void *tmp = sym_arr;
> + void *tmp;
> size += 16;
> - sym_arr = realloc(sym_arr, size * sizeof(struct symbol *));
> - if (!sym_arr) {
> - free(tmp);
> - return NULL;
> + tmp = realloc(sym_match_arr, size * sizeof(struct sym_match *));
> + if (!tmp) {
> + goto sym_re_search_free;
> }
Unnecessary curly braces (as reported by checkpatch.) Checkpatch reports
a few other issues BTW, you should run it and fix them.
> + sym_match_arr = tmp;
> }
> sym_calc_value(sym);
> - sym_arr[cnt++] = sym;
> + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
Cast not needed.
In fact I don't think this allocation is needed in the first place.
Calling malloc for every match is rather costly. If you would have
allocated an array of struct sym_match (rather than only pointers
thereto) before, you would not need this per-symbol malloc. Struct
sym_match is small enough to not warrant an extra level of allocation
and indirection IMHO.
> + 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
> + * 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;
> }
> - if (sym_arr)
> + if (sym_match_arr) {
> + 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[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);
> + }
> regfree(&re);
>
> return sym_arr;
--
Jean Delvare
Suse L3
Hi Yann,
Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <[email protected]>
>
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: "Yann E. MORIN" <[email protected]>
> Cc: Jean Delvare <[email protected]>
> Cc: Michal Marek <[email protected]>
> ---
> scripts/kconfig/mconf.c | 2 +-
> scripts/kconfig/nconf.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> index 6ee4aae..18d3dc9 100644
> --- a/scripts/kconfig/mconf.c
> +++ b/scripts/kconfig/mconf.c
> @@ -401,7 +401,7 @@ static void search_conf(void)
> struct subtitle_part stpart;
>
> title = str_new();
> - str_printf( &title, _("Enter %s (sub)string to search for "
> + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> "(with or without \"%s\")"), CONFIG_, CONFIG_);
While clearer, this also makes the title span over two lines when it
used to fit on a single one. I would like to suggest the following:
str_printf(&title, _("Enter (sub)string or regexp to search for "
"(with or without \"%s\")"), CONFIG_);
Rationale: the "(with or without CONFIG_)" makes things clear enough
IMHO, making the first occurrence redundant.
But if you don't like this proposal then let's just go with yours, it's
up to you.
>
> again:
> diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
> index 0183153..7975d8d 100644
> --- a/scripts/kconfig/nconf.c
> +++ b/scripts/kconfig/nconf.c
> @@ -695,7 +695,7 @@ static void search_conf(void)
> int dres;
>
> title = str_new();
> - str_printf( &title, _("Enter %s (sub)string to search for "
> + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> "(with or without \"%s\")"), CONFIG_, CONFIG_);
>
> again:
--
Jean Delvare
Suse L3
Jean, All,
On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > From: "Yann E. MORIN" <[email protected]>
[--SNIP--]
> > Since the search can be a regexp, it is possible that more than one symbol
> > matches exactly. In this case, we can't decide which to sort first, so we
> > fallback to alphabeticall sort.
>
> Typo: alphabetical.
OK.
> > Explain this (new!) sorting heuristic in the documentation.
>
> It's more of a rule than a heuristic.
OK.
> > Reported-by: Jean Delvare <[email protected]>
> > Signed-off-by: "Yann E. MORIN" <[email protected]>
> > Cc: Jean Delvare <[email protected]>
> > Cc: Michal Marek <[email protected]>
> > Cc: Roland Eggner <[email protected]>
> > Cc: Wang YanQing <[email protected]>
>
> I tested it and it works fine, thanks!
>
> Tested-by: Jean Delvare <[email protected]>
>
> Now comes my late review. Overall I like the idea and the code but a few
> things could be improved:
Since this is already in Michal's tree, on should I proceed?
- send an updated patch that replaces that one, or
- send another additional patch with your proposed changes?
> > + When searching, symbols are sorted thus:
> > + - exact match first: an exact match is when the search matches
> > + the complete symbol name;
>
> The use of singular seems to imply there can be only one, while there
> could be several as you explained above.
OK.
> > + - alphabetical order: when two symbols do not match exactly,
> > + they are sorted in alphabetical order (in the user's current
> > + locale).
>
> I think it would be better explained that way:
>
> - first, exact matches, sorted alphabetically (an exact match
> is when the search matches the complete symbol name);
> - then, other matches, sorted alphabetically.
OK.
> This is more concise and easier to grasp, methinks. I don't think the
> reference to the user's locale is needed, as there's no surprise here,
> and it probably doesn't matter anyway for kernel symbols.
Yes it may matter even for kernel symbols, since some locales may
consider '_' as a character to sort by, while other locales may not.
> BTW I was wondering if it would add value to explicitly print group
> header labels "Exact matches" and "Other matches" in the search results.
> What do you think?
It is not trivial to do, since the search function only returns a single
array, so there's no way for frontends (which do the display) to
differentiate which part of the array are exact matches, and which are
only partial matches.
It is much more involved, and I don't think it would be easy to
implement.
> > ______________________________________________________________________
> > User interface options for 'menuconfig'
> >
> > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> > index ab8f4c8..387d554 100644
> > --- a/scripts/kconfig/symbol.c
> > +++ b/scripts/kconfig/symbol.c
> > @@ -954,38 +954,98 @@ const char *sym_escape_string_value(const char *in)
> > return res;
> > }
> >
> > +struct sym_match {
> > + struct symbol *sym;
> > + off_t so, eo;
> > +};
> > +
> > +/* Compare matched symbols as thus:
> > + * - first, symbols that match exactly
> > + * - then, alphabetical sort
> > + */
> > +static int sym_rel_comp( const void *sym1, const void *sym2 )
>
> Coding style says no space after/before parentheses.
OK.
> > +{
> > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > + struct sym_match *s2 = *(struct sym_match **)sym2;
>
> You shouldn't need these casts.
Probably not, indeed, but I like to write (and read) what I expect to
happen, and pointer arithmetics is always something I dread to foobar.
OK.
> > + int l1, l2;
> > +
> > + /* Exact match:
> > + * - if matched length on symbol s1 is the length of that symbol,
> > + * then this symbol should come first;
> > + * - if matched length on symbol s2 is the length of that symbol,
> > + * then this symbol should come first.
> > + * Note: since the search can be a regexp, both symbols may match
> > + * 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))
> > + return -1;
> > + if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
> > + return 1;
>
> Strlen is expensive so I would avoid calling it twice on the same symbol
> name. You could store the result, together with the result of the
> comparison, while you're at it. Something like:
>
> int exact1, exact2;
>
> exact1 = l1 == strlen(s1->sym->name);
> exact2 = l2 == strlen(s2->sym->name);
> if (exact1 && !exact2)
> return -1;
> if (!exact1 && exact2)
> return 1;
>
> You may even no longer need to introduce l1 and l2 as you would use them
> only once.
OK.
> It might be even faster to store the symbol length in struct sym_match,
> but this would increase the structure size and consequently memory
> consumption, and it is questionable if the speed gain is worth it.
> Probably not.
I intended this structure to only hold the result of the regexp match,
and nothing more. The symbol length does not belong there, IMHO.
Besides, it's easy to get back, since the symbol struct is available.
OTOH, we would gain by computing strlen at regexp match, instead of
every time in the sorting function.
But that's micro-optimisation, methinks. Searching for the example
^ATH.K took less than me focusing from the RETURN key to the screen. ;-)
[--SNIP--]
> > for_all_symbols(i, sym) {
> > + struct sym_match *tmp_sym_match;
> > if (sym->flags & SYMBOL_CONST || !sym->name)
> > continue;
> > - if (regexec(&re, sym->name, 0, NULL, 0))
> > + if (regexec(&re, sym->name, 1, match, 0))
> > continue;
> > if (cnt + 1 >= size) {
>
> I think the "+ 1" can be dropped because the new array is not
> NULL-terminated.
I'll look into that.
> > - void *tmp = sym_arr;
> > + void *tmp;
> > size += 16;
> > - sym_arr = realloc(sym_arr, size * sizeof(struct symbol *));
> > - if (!sym_arr) {
> > - free(tmp);
> > - return NULL;
> > + tmp = realloc(sym_match_arr, size * sizeof(struct sym_match *));
> > + if (!tmp) {
> > + goto sym_re_search_free;
> > }
>
> Unnecessary curly braces (as reported by checkpatch.) Checkpatch reports
> a few other issues BTW, you should run it and fix them.
My fault, as I always use curly braces, even around single statements,
in many other projects. ( /me believes this kernel coding style is bad,
but will abide by the rules! ;-) ).
OK.
> > + sym_match_arr = tmp;
> > }
> > sym_calc_value(sym);
> > - sym_arr[cnt++] = sym;
> > + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
>
> Cast not needed.
OK.
> In fact I don't think this allocation is needed in the first place.
> Calling malloc for every match is rather costly. If you would have
> allocated an array of struct sym_match (rather than only pointers
> thereto) before, you would not need this per-symbol malloc. Struct
> sym_match is small enough to not warrant an extra level of allocation
> and indirection IMHO.
OK, will look into that.
Thank you for the review! :-)
Regards,
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. |
'------------------------------^-------^------------------^--------------------'
Jean, All,
On 2013-07-08 13:25 +0200, Jean Delvare spake thusly:
> Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > From: "Yann E. MORIN" <[email protected]>
> >
> > Reported-by: Jean Delvare <[email protected]>
> > Signed-off-by: "Yann E. MORIN" <[email protected]>
> > Cc: Jean Delvare <[email protected]>
> > Cc: Michal Marek <[email protected]>
> > ---
> > scripts/kconfig/mconf.c | 2 +-
> > scripts/kconfig/nconf.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > index 6ee4aae..18d3dc9 100644
> > --- a/scripts/kconfig/mconf.c
> > +++ b/scripts/kconfig/mconf.c
> > @@ -401,7 +401,7 @@ static void search_conf(void)
> > struct subtitle_part stpart;
> >
> > title = str_new();
> > - str_printf( &title, _("Enter %s (sub)string to search for "
> > + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> > "(with or without \"%s\")"), CONFIG_, CONFIG_);
>
> While clearer, this also makes the title span over two lines when it
> used to fit on a single one. I would like to suggest the following:
>
> str_printf(&title, _("Enter (sub)string or regexp to search for "
> "(with or without \"%s\")"), CONFIG_);
>
> Rationale: the "(with or without CONFIG_)" makes things clear enough
> IMHO, making the first occurrence redundant.
OK, I'll send another patch with the text updated with your suggestion.
Thank you! :-)
Regards,
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. |
'------------------------------^-------^------------------^--------------------'
Michal, All,
On 2013-07-08 19:35 +0200, Yann E. MORIN spake thusly:
> On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > From: "Yann E. MORIN" <[email protected]>
> [--SNIP--]
> > > Since the search can be a regexp, it is possible that more than one symbol
> > > matches exactly. In this case, we can't decide which to sort first, so we
> > > fallback to alphabeticall sort.
[--SNIP--]
> > > Reported-by: Jean Delvare <[email protected]>
> > > Signed-off-by: "Yann E. MORIN" <[email protected]>
> > > Cc: Jean Delvare <[email protected]>
> > > Cc: Michal Marek <[email protected]>
> > > Cc: Roland Eggner <[email protected]>
> > > Cc: Wang YanQing <[email protected]>
> >
> > I tested it and it works fine, thanks!
> >
> > Tested-by: Jean Delvare <[email protected]>
> >
> > Now comes my late review. Overall I like the idea and the code but a few
> > things could be improved:
>
> Since this is already in Michal's tree, on should I proceed?
> - send an updated patch that replaces that one, or
> - send another additional patch with your proposed changes?
OK, since Michal already sent his pull-request to Linus, I'll prepare a
corrective patch I'll submit before the end of the week. Is that OK with
you, Michal?
[--SNIP--]
> > > +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;
> >
> > You shouldn't need these casts.
>
> Probably not, indeed, but I like to write (and read) what I expect to
> happen, and pointer arithmetics is always something I dread to foobar.
In fact, we need the cast, otherwise gcc whines about const/non-const.
[--SNIP--]
> > > for_all_symbols(i, sym) {
> > > + struct sym_match *tmp_sym_match;
> > > if (sym->flags & SYMBOL_CONST || !sym->name)
> > > continue;
> > > - if (regexec(&re, sym->name, 0, NULL, 0))
> > > + if (regexec(&re, sym->name, 1, match, 0))
> > > continue;
> > > if (cnt + 1 >= size) {
> >
> > I think the "+ 1" can be dropped because the new array is not
> > NULL-terminated.
Indeed.
> > > + sym_match_arr = tmp;
> > > }
> > > sym_calc_value(sym);
> > > - sym_arr[cnt++] = sym;
> > > + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
> >
> > Cast not needed.
>
> OK.
>
> > In fact I don't think this allocation is needed in the first place.
> > Calling malloc for every match is rather costly. If you would have
> > allocated an array of struct sym_match (rather than only pointers
> > thereto) before, you would not need this per-symbol malloc. Struct
> > sym_match is small enough to not warrant an extra level of allocation
> > and indirection IMHO.
Indeed, it makes for cleaner code.
Thank you again! :-)
Regards,
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. |
'------------------------------^-------^------------------^--------------------'
Hi Yann,
Sorry again for the late reply, busy...
Le Monday 08 July 2013 à 19:35 +0200, Yann E. MORIN a écrit :
> On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > This is more concise and easier to grasp, methinks. I don't think the
> > reference to the user's locale is needed, as there's no surprise here,
> > and it probably doesn't matter anyway for kernel symbols.
>
> Yes it may matter even for kernel symbols, since some locales may
> consider '_' as a character to sort by, while other locales may not.
I didn't know that, thanks for the information. That being said I still
believe mentioning the user's locale is not needed ;-)
> > BTW I was wondering if it would add value to explicitly print group
> > header labels "Exact matches" and "Other matches" in the search results.
> > What do you think?
>
> It is not trivial to do, since the search function only returns a single
> array, so there's no way for frontends (which do the display) to
> differentiate which part of the array are exact matches, and which are
> only partial matches.
>
> It is much more involved, and I don't think it would be easy to
> implement.
I understand, and I agree it's not worth the effort.
> > > +{
> > > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > > + struct sym_match *s2 = *(struct sym_match **)sym2;
> >
> > You shouldn't need these casts.
>
> Probably not, indeed, but I like to write (and read) what I expect to
> happen, and pointer arithmetics is always something I dread to foobar.
I hear this argument every now and then but I do not think it holds. If
you always forcibly cast pointers even when you don't have to, then gcc
has no chance to warn you if you get it wrong.
More on this later as I reply to your next post...
> > It might be even faster to store the symbol length in struct sym_match,
> > but this would increase the structure size and consequently memory
> > consumption, and it is questionable if the speed gain is worth it.
> > Probably not.
>
> I intended this structure to only hold the result of the regexp match,
> and nothing more. The symbol length does not belong there, IMHO.
> Besides, it's easy to get back, since the symbol struct is available.
>
> OTOH, we would gain by computing strlen at regexp match, instead of
> every time in the sorting function.
>
> But that's micro-optimisation, methinks. Searching for the example
> ^ATH.K took less than me focusing from the RETURN key to the screen. ;-)
OK, fair enough. I always pay attention to algorithmic complexity
because not everyone is running brand new and powerful machines (I am
not, to start with.)
I agree that the number of items returned by the search should generally
be small enough so it shouldn't be an issue in practice. If you want to
test your code on extreme cases though, you could search for "A" or even
".". It takes about 20 seconds for "A" on my machine and close to 1
minute for ".", which is quite slow. That being said that was already
the case before your patch, so I'm not blaming your changes for it.
> (...)
> Thank you for the review! :-)
You're welcome :-)
--
Jean Delvare
Suse L3
Yann, Michal,
Le Wednesday 10 July 2013 à 22:46 +0200, Yann E. MORIN a écrit :
> Michal, All,
>
> On 2013-07-08 19:35 +0200, Yann E. MORIN spake thusly:
> > On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > > From: "Yann E. MORIN" <[email protected]>
> > [--SNIP--]
> > > > Since the search can be a regexp, it is possible that more than one symbol
> > > > matches exactly. In this case, we can't decide which to sort first, so we
> > > > fallback to alphabeticall sort.
> [--SNIP--]
> > > > Reported-by: Jean Delvare <[email protected]>
> > > > Signed-off-by: "Yann E. MORIN" <[email protected]>
> > > > Cc: Jean Delvare <[email protected]>
> > > > Cc: Michal Marek <[email protected]>
> > > > Cc: Roland Eggner <[email protected]>
> > > > Cc: Wang YanQing <[email protected]>
> > >
> > > I tested it and it works fine, thanks!
> > >
> > > Tested-by: Jean Delvare <[email protected]>
> > >
> > > Now comes my late review. Overall I like the idea and the code but a few
> > > things could be improved:
> >
> > Since this is already in Michal's tree, on should I proceed?
> > - send an updated patch that replaces that one, or
> > - send another additional patch with your proposed changes?
>
> OK, since Michal already sent his pull-request to Linus, I'll prepare a
> corrective patch I'll submit before the end of the week. Is that OK with
> you, Michal?
Sounds good to me at least. I'll do my best to test and review it
quickly this time.
> [--SNIP--]
> > > > +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;
> > >
> > > You shouldn't need these casts.
> >
> > Probably not, indeed, but I like to write (and read) what I expect to
> > happen, and pointer arithmetics is always something I dread to foobar.
>
> In fact, we need the cast, otherwise gcc whines about const/non-const.
And quite rightly so, as you are taking const pointers (i.e. the caller
told you you are _not_ allowed to change the contents) and making them
non-const pointers (i.e. you give yourself the right to still change the
contents.) It happens that your function doesn't actually change the
contents, so no harm done, but this is still conceptually wrong.
Preserving the const nature of pointers down the call chain lets the
compiler warn you if a function changes data it was not supposed to.
So what you want to do is:
static int sym_rel_comp(const void *sym1, const void *sym2)
{
const struct sym_match *s1 = sym1;
const struct sym_match *s2 = sym2;
This is both concise and correct, and it makes gcc happy.
Thanks,
--
Jean Delvare
Suse L3
Jean, All,
On 2013-07-12 11:07 +0200, Jean Delvare spake thusly:
[--SNIP--]
> > > > > +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;
> > > >
> > > > You shouldn't need these casts.
> > >
> > > Probably not, indeed, but I like to write (and read) what I expect to
> > > happen, and pointer arithmetics is always something I dread to foobar.
> >
> > In fact, we need the cast, otherwise gcc whines about const/non-const.
>
> And quite rightly so, as you are taking const pointers (i.e. the caller
> told you you are _not_ allowed to change the contents) and making them
> non-const pointers (i.e. you give yourself the right to still change the
> contents.) It happens that your function doesn't actually change the
> contents, so no harm done, but this is still conceptually wrong.
> Preserving the const nature of pointers down the call chain lets the
> compiler warn you if a function changes data it was not supposed to.
>
> So what you want to do is:
>
> static int sym_rel_comp(const void *sym1, const void *sym2)
> {
> const struct sym_match *s1 = sym1;
> const struct sym_match *s2 = sym2;
>
> This is both concise and correct, and it makes gcc happy.
Yes, that's what I thought to, and what I was about to do. Thanks for
confirming this! :-)
Regards,
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. |
'------------------------------^-------^------------------^--------------------'
Hi Yann,
Le Monday 08 July 2013 à 19:37 +0200, Yann E. MORIN a écrit :
> Jean, All,
>
> On 2013-07-08 13:25 +0200, Jean Delvare spake thusly:
> > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > From: "Yann E. MORIN" <[email protected]>
> > >
> > > Reported-by: Jean Delvare <[email protected]>
> > > Signed-off-by: "Yann E. MORIN" <[email protected]>
> > > Cc: Jean Delvare <[email protected]>
> > > Cc: Michal Marek <[email protected]>
> > > ---
> > > scripts/kconfig/mconf.c | 2 +-
> > > scripts/kconfig/nconf.c | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > > index 6ee4aae..18d3dc9 100644
> > > --- a/scripts/kconfig/mconf.c
> > > +++ b/scripts/kconfig/mconf.c
> > > @@ -401,7 +401,7 @@ static void search_conf(void)
> > > struct subtitle_part stpart;
> > >
> > > title = str_new();
> > > - str_printf( &title, _("Enter %s (sub)string to search for "
> > > + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> > > "(with or without \"%s\")"), CONFIG_, CONFIG_);
> >
> > While clearer, this also makes the title span over two lines when it
> > used to fit on a single one. I would like to suggest the following:
> >
> > str_printf(&title, _("Enter (sub)string or regexp to search for "
> > "(with or without \"%s\")"), CONFIG_);
> >
> > Rationale: the "(with or without CONFIG_)" makes things clear enough
> > IMHO, making the first occurrence redundant.
>
> OK, I'll send another patch with the text updated with your suggestion.
Did you? I can't remember seeing it.
--
Jean Delvare
Suse L3
Jean, All,
On Tuesday 16 July 2013 16:31:20 Jean Delvare wrote:
> Le Monday 08 July 2013 à 19:37 +0200, Yann E. MORIN a écrit :
> > On 2013-07-08 13:25 +0200, Jean Delvare spake thusly:
> > > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
[--SNIP--]
> > > > - str_printf( &title, _("Enter %s (sub)string to search for "
> > > > + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> > > > "(with or without \"%s\")"), CONFIG_, CONFIG_);
> > >
> > > While clearer, this also makes the title span over two lines when it
> > > used to fit on a single one. I would like to suggest the following:
> > >
> > > str_printf(&title, _("Enter (sub)string or regexp to search for "
> > > "(with or without \"%s\")"), CONFIG_);
> > >
> > > Rationale: the "(with or without CONFIG_)" makes things clear enough
> > > IMHO, making the first occurrence redundant.
> >
> > OK, I'll send another patch with the text updated with your suggestion.
>
> Did you? I can't remember seeing it.
Doh! I forgot that one... Will do.
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. |
'------------------------------'-------'------------------'--------------------'