| Message ID | 1442257543-6194-2-git-send-email-petr.vorel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ma, 2015-09-14 at 21:05 +0200, Petr Vorel wrote: > From: Paul Bolle <pebolle@tiscali.nl> (Petr and I have been in contact about this patch. The way it's been resubmitted is a bit odd. That's probably because I didn't give Petr's queries about my patch the attention they deserved.) > This is a _hack_ to show what might be a better way to do this. That's the reason I submitted this as an RFC. (By now I doubt whether RFC patches are worth the effort. I'm inclined to think that one might as well send in actual patches. Reviewing RFCs seems to be about as much work as reviewing real patches. So why bother reviewing them when the submitter might claim "It's only an RFC!" once things get serious?) > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> I did this for a reason! > Tested-by: Petr Vorel <petr.vorel@gmail.com> (Resubmitting someone else's patch can be the right thing to do. The patch should be correctly signed off in that case - this patch wasn't! - and the person resubmitting should sign off too. Petr didn't sign off on this patch.) > +static void rev_dep_gstr_print(struct expr *e, struct gstr *gs) > +{ > + struct gstr tmp = str_new(); > + const char *prev, *start; > + char *beam; > + > + expr_gstr_print(e, &tmp); This is the main problem with my RFC. I feel that the right thing for rev_dep_gstr_print() would be to work on a struct expr directly, instead of cheating by using expr_gstr_print()'s output. Would you mind working on a v2 that tries to do that? > + prev = start = str_get(&tmp); > + > + str_append(gs, "\n - "); > + > + while ((beam = index(start, '|'))) { > + char *lparen = index(start, '('); > + > + /* don't split "(I || J)" */ > + if (lparen && (lparen < beam)) { > + const char *rparen = matching_paren(++lparen); > + > + /* skip the expression inside parentheses */ > + start = ++rparen; > + continue; > + } > + > + /* we can assume we're fed a sane string, so the space before > + * the beam gets turned into a NUL */ > + *(beam - 1) = '\0'; > + str_append(gs, prev); > + str_append(gs, "\n - "); > + /* assume sane string, so skip the second beam */ > + beam++; > + /* trim */ > + while (*++beam == ' ') > + ; > + prev = start = beam; > + } > + > + str_append(gs, prev); > + > + str_free(&tmp); > +} Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dne 14.9.2015 v 21:05 Petr Vorel napsal(a): > From: Paul Bolle <pebolle@tiscali.nl> > > rev_dep expressions can get rather unwieldy, especially if a symbol is > selected by more than a handful of other symbols. Ie, it's possible to > have near endless expressions like: > A && B && !C || D || F && (G || H) || [...] > > Chop these expressions into actually readable chunks: > - A && B && !C > - D > - F && (G || H) > - [...] > > Ie, transform the top level "||" tokens into newlines and prepend each > line with a minus. This makes the "Selected by:" blurb much easier to > read. > --- > Today I found myself wondering why a certain Kconfig was selected. > Currently menuconfig's help is of no use in complicated cases. Please > look at the help of USB or CRYPTO to see what I mean. > > This is a _hack_ to show what might be a better way to do this. It > parses a stringified version of the reverse dependency, and not the > actual reverse dependecy expression. But that was easier to cobble > together. > > One cool improvement would be to change to minus in front of the > subexpressions to Y or M for those that actually set the symbol. Anyhow, > other suggestions and feedback is welcome. > > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> > Tested-by: Petr Vorel <petr.vorel@gmail.com> > Cc: Paul Bolle <pebolle@tiscali.nl> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Petr Vorel <petr.vorel@gmail.com> > --- > scripts/kconfig/menu.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 81 insertions(+), 2 deletions(-) > > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c > index b05cc3d..a8b37a2 100644 > --- a/scripts/kconfig/menu.c > +++ b/scripts/kconfig/menu.c > @@ -613,6 +613,86 @@ static struct property *get_symbol_prop(struct symbol *sym) > } > > /* > + * Assuming we're just past an opening parenthesis in a NUL terminated string, > + * find it's closing parenthesis and return its postion. Die otherwise. > + */ > +static const char *matching_paren(const char *s) > +{ > + int lvl = 1; > + > + while (1) { > + if (*s == '(') > + lvl++; > + else if (*s == ')') > + lvl--; > + if (lvl == 0) > + break; > + if (*s == '\0') > + /* huh? */ > + exit(1); assert/abort? BTW, the whole splitting can be done in a singe loop a-la while (c = s++) { if (c == '(') lvl++; else if (c == ')') lvl--; else if (c == '|' && lvl == 0) /* insert newline and indent */ /* output c */ } No need to seek back and forth in the string. > - expr_gstr_print(sym->rev_dep.expr, r); > - str_append(r, "\n"); > + rev_dep_gstr_print(sym->rev_dep.expr, r); ... and a cleaner fix would be to patch the printer, instead of generating a string representation and converting it. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On ma, 2015-10-26 at 21:21 +0100, Michal Marek wrote: > ... and a cleaner fix would be to patch the printer, instead of > generating a string representation and converting it. Petr has submitted a v2 that appears to do just that. (Actually we're already at v3, see http://www.spinics.net/lists/linux-kbuild/msg11840.html .) I haven't looked at the patch too closely (until now I mostly helped Petr with the boring stuff, like correct sign-off). Perhaps I find time to have a close look at v3. Whatever happens, it's rather nice to notice that Petr stopped (politely!) bugging me to finish my RFC patch and submitted an updated version. I really appreciate that! Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index b05cc3d..a8b37a2 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -613,6 +613,86 @@ static struct property *get_symbol_prop(struct symbol *sym) } /* + * Assuming we're just past an opening parenthesis in a NUL terminated string, + * find it's closing parenthesis and return its postion. Die otherwise. + */ +static const char *matching_paren(const char *s) +{ + int lvl = 1; + + while (1) { + if (*s == '(') + lvl++; + else if (*s == ')') + lvl--; + if (lvl == 0) + break; + if (*s == '\0') + /* huh? */ + exit(1); + s++; + } + + return s; +} + +/* + * rev_dep expressions can get rather unwieldy, especially if a symbol is + * selected by more than a handful of other symbols. Ie, it's possible to + * have near endless expressions like: + * A && B && !C || D || F && (G || H) || [...] + * + * Chop these expressions into actually readable chunks: + * - A && B && !C + * - D + * - F && (G || H) + * - [...] + * + * Ie, transform the top level "||" tokens into newlines and prepend each line + * with a minus. This makes the "Selected by:" blurb much easier to read. + */ +static void rev_dep_gstr_print(struct expr *e, struct gstr *gs) +{ + struct gstr tmp = str_new(); + const char *prev, *start; + char *beam; + + expr_gstr_print(e, &tmp); + prev = start = str_get(&tmp); + + str_append(gs, "\n - "); + + while ((beam = index(start, '|'))) { + char *lparen = index(start, '('); + + /* don't split "(I || J)" */ + if (lparen && (lparen < beam)) { + const char *rparen = matching_paren(++lparen); + + /* skip the expression inside parentheses */ + start = ++rparen; + continue; + } + + /* we can assume we're fed a sane string, so the space before + * the beam gets turned into a NUL */ + *(beam - 1) = '\0'; + str_append(gs, prev); + str_append(gs, "\n - "); + /* assume sane string, so skip the second beam */ + beam++; + /* trim */ + while (*++beam == ' ') + ; + prev = start = beam; + } + + str_append(gs, prev); + + str_free(&tmp); +} + +/* * head is optional and may be NULL */ static void get_symbol_str(struct gstr *r, struct symbol *sym, @@ -661,8 +741,7 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym, str_append(r, "\n"); if (sym->rev_dep.expr) { str_append(r, _(" Selected by: ")); - expr_gstr_print(sym->rev_dep.expr, r); - str_append(r, "\n"); + rev_dep_gstr_print(sym->rev_dep.expr, r); } str_append(r, "\n\n"); }