Many of the variable names in scripts/kconfig/symbol.c are
far too terse to the point of not at all identifying _what_
they are actually used for (`p` and `l` as a couple examples),
and overall there is a large amount of code that could use
some cleaning up.
Give more explicit names to these variables, fix a couple cases
where different variables were sharing the same name and shadowing
each other, and overall cleanup a bit of the messiness in
sym_expand_string_value() and sym_escape_string_value()
while maintaining equivalent program behavior.
Suggested-by: Ulf Magnusson <[email protected]>
Signed-off-by: Joey Pabalinas <[email protected]>
1 file changed, 69 insertions(+), 61 deletions(-)
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 2220bc4b051bd914e3..9ee32ddb44e193719c 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -5,8 +5,8 @@
#include <ctype.h>
#include <stdlib.h>
-#include <string.h>
#include <regex.h>
+#include <linux/string.h>
#include <sys/utsname.h>
#include "lkc.h"
@@ -337,7 +337,7 @@ void sym_calc_value(struct symbol *sym)
{
struct symbol_value newval, oldval;
struct property *prop;
- struct expr *e;
+ struct expr *expr;
if (!sym)
return;
@@ -469,7 +469,7 @@ void sym_calc_value(struct symbol *sym)
struct symbol *choice_sym;
prop = sym_get_choice_prop(sym);
- expr_list_for_each_sym(prop->expr, e, choice_sym) {
+ expr_list_for_each_sym(prop->expr, expr, choice_sym) {
if ((sym->flags & SYMBOL_WRITE) &&
choice_sym->visible != no)
choice_sym->flags |= SYMBOL_WRITE;
@@ -899,94 +899,100 @@ struct symbol *sym_find(const char *name)
* name to be expanded shall be prefixed by a '$'. Unknown symbol expands to
* the empty string.
*/
-char *sym_expand_string_value(const char *in)
+char *sym_expand_string_value(const char *src)
{
- const char *src;
- char *res;
- size_t reslen;
+ const char *in;
+ char *res, *out;
+ size_t res_len, src_len;
/*
- * Note: 'in' might come from a token that's about to be
+ * Note: 'src' might come from a token that'src about to be
* freed, so make sure to always allocate a new string
*/
- reslen = strlen(in) + 1;
- res = xmalloc(reslen);
- res[0] = '\0';
+ res_len = strlen(src) + 1;
+ res = xmalloc(res_len);
+ out = res;
- while ((src = strchr(in, '$'))) {
+ while ((in = strchr(src, '$'))) {
char *p, name[SYMBOL_MAXLENGTH];
- const char *symval = "";
+ const char *sym_val = "";
struct symbol *sym;
- size_t newlen;
+ size_t new_len, sym_len;
- strncat(res, in, src - in);
- src++;
+ strscpy(out, src, in - src);
+ out += in - src;
+ in++;
p = name;
- while (isalnum(*src) || *src == '_')
- *p++ = *src++;
+ while (isalnum(*in) || *in == '_')
+ *p++ = *in++;
*p = '\0';
sym = sym_find(name);
if (sym != NULL) {
sym_calc_value(sym);
- symval = sym_get_string_value(sym);
+ sym_val = sym_get_string_value(sym);
}
- newlen = strlen(res) + strlen(symval) + strlen(src) + 1;
- if (newlen > reslen) {
- reslen = newlen;
- res = xrealloc(res, reslen);
+ sym_len = strlen(sym_val);
+ new_len = sym_len + strlen(res) + strlen(in) + 1;
+ if (new_len > res_len) {
+ res_len = new_len;
+ res = xrealloc(res, res_len);
}
- strcat(res, symval);
- in = src;
+ strscpy(out, sym_val, sym_len);
+ out += sym_len;
+ src = in;
}
- strcat(res, in);
+ src_len = strlen(src);
+ strscpy(out, src, src_len);
+ out += src_len;
+ *out = '\0';
return res;
}
-const char *sym_escape_string_value(const char *in)
+const char *sym_escape_string_value(const char *src)
{
- const char *p;
- size_t reslen;
- char *res;
- size_t l;
+ const char *in;
+ size_t res_len, in_len;
+ char *res, *out;
- reslen = strlen(in) + strlen("\"\"") + 1;
+ res_len = strlen(src) + strlen("\"\"") + 1;
- p = in;
+ in = src;
for (;;) {
- l = strcspn(p, "\"\\");
- p += l;
+ in_len = strcspn(in, "\"\\");
+ in += in_len;
- if (p[0] == '\0')
+ if (*in == '\0')
break;
- reslen++;
- p++;
+ res_len++;
+ in++;
}
- res = xmalloc(reslen);
- res[0] = '\0';
+ res = xmalloc(res_len);
+ out = res;
+ *out++ = '\"';
- strcat(res, "\"");
-
- p = in;
+ in = src;
for (;;) {
- l = strcspn(p, "\"\\");
- strncat(res, p, l);
- p += l;
+ in_len = strcspn(in, "\"\\");
+ strscpy(out, in, in_len);
+ in += in_len;
+ out += in_len;
- if (p[0] == '\0')
+ if (*in == '\0')
break;
- strcat(res, "\\");
- strncat(res, p++, 1);
+ *out++ = '\\';
+ *out++ = *in++;
}
+ *out++ = '\"';
+ *out = '\0';
- strcat(res, "\"");
return res;
}
@@ -1014,8 +1020,8 @@ static int sym_rel_comp(const void *sym1, const void *sym2)
* exactly; if this is the case, we can't decide which comes first,
* and we fallback to sorting alphabetically.
*/
- exact1 = (s1->eo - s1->so) == strlen(s1->sym->name);
- exact2 = (s2->eo - s2->so) == strlen(s2->sym->name);
+ exact1 = (s1->eo - s1->so) == (off_t)strlen(s1->sym->name);
+ exact2 = (s2->eo - s2->so) == (off_t)strlen(s2->sym->name);
if (exact1 && !exact2)
return -1;
if (!exact1 && exact2)
@@ -1390,31 +1396,33 @@ const char *prop_get_type_name(enum prop_type type)
return "unknown";
}
-static void prop_add_env(const char *env)
+static void prop_add_env(const char *env_key)
{
struct symbol *sym, *sym2;
struct property *prop;
- char *p;
+ char *env_val;
sym = current_entry->sym;
sym->flags |= SYMBOL_AUTO;
for_all_properties(sym, prop, P_ENV) {
sym2 = prop_get_symbol(prop);
- if (strcmp(sym2->name, env))
+ if (strcmp(sym2->name, env_key))
menu_warn(current_entry, "redefining environment symbol from %s",
sym2->name);
return;
}
prop = prop_alloc(P_ENV, sym);
- prop->expr = expr_alloc_symbol(sym_lookup(env, SYMBOL_CONST));
+ prop->expr = expr_alloc_symbol(sym_lookup(env_key, SYMBOL_CONST));
sym_env_list = expr_alloc_one(E_LIST, sym_env_list);
sym_env_list->right.sym = sym;
+ env_val = getenv(env_key);
- p = getenv(env);
- if (p)
- sym_add_default(sym, p);
- else
- menu_warn(current_entry, "environment variable %s undefined", env);
+ if (likely(env_val)) {
+ sym_add_default(sym, env_val);
+ return;
+ }
+
+ menu_warn(current_entry, "environment variable %s undefined", env_key);
}
--
2.16.2
2018-03-10 19:56 GMT+09:00 Joey Pabalinas <[email protected]>:
> Many of the variable names in scripts/kconfig/symbol.c are
> far too terse to the point of not at all identifying _what_
> they are actually used for (`p` and `l` as a couple examples),
> and overall there is a large amount of code that could use
> some cleaning up.
>
> Give more explicit names to these variables, fix a couple cases
> where different variables were sharing the same name and shadowing
> each other, and overall cleanup a bit of the messiness in
> sym_expand_string_value() and sym_escape_string_value()
> while maintaining equivalent program behavior.
>
> Suggested-by: Ulf Magnusson <[email protected]>
> Signed-off-by: Joey Pabalinas <[email protected]>
>
> 1 file changed, 69 insertions(+), 61 deletions(-)
First of all, I can not compile Kconfig with this patch.
masahiro@grover:~/workspace/linux$ make defconfig
HOSTLD scripts/kconfig/conf
scripts/kconfig/zconf.tab.o: In function `sym_expand_string_value':
zconf.tab.c:(.text+0x81a2): undefined reference to `strscpy'
zconf.tab.c:(.text+0x8287): undefined reference to `strscpy'
zconf.tab.c:(.text+0x82b5): undefined reference to `strscpy'
scripts/kconfig/zconf.tab.o: In function `sym_escape_string_value':
zconf.tab.c:(.text+0x869b): undefined reference to `strscpy'
scripts/kconfig/zconf.tab.o: In function `menu_add_option':
zconf.tab.c:(.text+0x99b7): undefined reference to `likely'
collect2: error: ld returned 1 exit status
scripts/Makefile.host:99: recipe for target 'scripts/kconfig/conf' failed
make[1]: *** [scripts/kconfig/conf] Error 1
Makefile:512: recipe for target 'defconfig' failed
make: *** [defconfig] Error 2
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 2220bc4b051bd914e3..9ee32ddb44e193719c 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -5,8 +5,8 @@
>
> #include <ctype.h>
> #include <stdlib.h>
> -#include <string.h>
> #include <regex.h>
> +#include <linux/string.h>
> #include <sys/utsname.h>
<string.h> is correct.
I want to see Kconfig improvements in a bigger picture.
The changes below are noise.
> #include "lkc.h"
> @@ -337,7 +337,7 @@ void sym_calc_value(struct symbol *sym)
> {
> struct symbol_value newval, oldval;
> struct property *prop;
> - struct expr *e;
> + struct expr *expr;
>
> if (!sym)
> return;
> @@ -469,7 +469,7 @@ void sym_calc_value(struct symbol *sym)
> struct symbol *choice_sym;
>
> prop = sym_get_choice_prop(sym);
> - expr_list_for_each_sym(prop->expr, e, choice_sym) {
> + expr_list_for_each_sym(prop->expr, expr, choice_sym) {
> if ((sym->flags & SYMBOL_WRITE) &&
> choice_sym->visible != no)
> choice_sym->flags |= SYMBOL_WRITE;
> @@ -899,94 +899,100 @@ struct symbol *sym_find(const char *name)
> * name to be expanded shall be prefixed by a '$'. Unknown symbol expands to
> * the empty string.
> */
> -char *sym_expand_string_value(const char *in)
> +char *sym_expand_string_value(const char *src)
> {
> - const char *src;
> - char *res;
> - size_t reslen;
> + const char *in;
> + char *res, *out;
> + size_t res_len, src_len;
>
> /*
> - * Note: 'in' might come from a token that's about to be
> + * Note: 'src' might come from a token that'src about to be
> * freed, so make sure to always allocate a new string
> */
> - reslen = strlen(in) + 1;
> - res = xmalloc(reslen);
> - res[0] = '\0';
> + res_len = strlen(src) + 1;
> + res = xmalloc(res_len);
> + out = res;
>
> - while ((src = strchr(in, '$'))) {
> + while ((in = strchr(src, '$'))) {
> char *p, name[SYMBOL_MAXLENGTH];
> - const char *symval = "";
> + const char *sym_val = "";
> struct symbol *sym;
> - size_t newlen;
> + size_t new_len, sym_len;
>
> - strncat(res, in, src - in);
> - src++;
> + strscpy(out, src, in - src);
> + out += in - src;
> + in++;
>
> p = name;
> - while (isalnum(*src) || *src == '_')
> - *p++ = *src++;
> + while (isalnum(*in) || *in == '_')
> + *p++ = *in++;
> *p = '\0';
>
> sym = sym_find(name);
> if (sym != NULL) {
> sym_calc_value(sym);
> - symval = sym_get_string_value(sym);
> + sym_val = sym_get_string_value(sym);
> }
>
> - newlen = strlen(res) + strlen(symval) + strlen(src) + 1;
> - if (newlen > reslen) {
> - reslen = newlen;
> - res = xrealloc(res, reslen);
> + sym_len = strlen(sym_val);
> + new_len = sym_len + strlen(res) + strlen(in) + 1;
> + if (new_len > res_len) {
> + res_len = new_len;
> + res = xrealloc(res, res_len);
> }
>
> - strcat(res, symval);
> - in = src;
> + strscpy(out, sym_val, sym_len);
> + out += sym_len;
> + src = in;
> }
> - strcat(res, in);
> + src_len = strlen(src);
> + strscpy(out, src, src_len);
> + out += src_len;
> + *out = '\0';
>
> return res;
> }
>
> -const char *sym_escape_string_value(const char *in)
> +const char *sym_escape_string_value(const char *src)
> {
> - const char *p;
> - size_t reslen;
> - char *res;
> - size_t l;
> + const char *in;
> + size_t res_len, in_len;
> + char *res, *out;
>
> - reslen = strlen(in) + strlen("\"\"") + 1;
> + res_len = strlen(src) + strlen("\"\"") + 1;
>
> - p = in;
> + in = src;
> for (;;) {
> - l = strcspn(p, "\"\\");
> - p += l;
> + in_len = strcspn(in, "\"\\");
> + in += in_len;
>
> - if (p[0] == '\0')
> + if (*in == '\0')
> break;
>
> - reslen++;
> - p++;
> + res_len++;
> + in++;
> }
>
> - res = xmalloc(reslen);
> - res[0] = '\0';
> + res = xmalloc(res_len);
> + out = res;
> + *out++ = '\"';
>
> - strcat(res, "\"");
> -
> - p = in;
> + in = src;
> for (;;) {
> - l = strcspn(p, "\"\\");
> - strncat(res, p, l);
> - p += l;
> + in_len = strcspn(in, "\"\\");
> + strscpy(out, in, in_len);
> + in += in_len;
> + out += in_len;
>
> - if (p[0] == '\0')
> + if (*in == '\0')
> break;
>
> - strcat(res, "\\");
> - strncat(res, p++, 1);
> + *out++ = '\\';
> + *out++ = *in++;
> }
> + *out++ = '\"';
> + *out = '\0';
>
> - strcat(res, "\"");
> return res;
> }
>
> @@ -1014,8 +1020,8 @@ static int sym_rel_comp(const void *sym1, const void *sym2)
> * exactly; if this is the case, we can't decide which comes first,
> * and we fallback to sorting alphabetically.
> */
> - exact1 = (s1->eo - s1->so) == strlen(s1->sym->name);
> - exact2 = (s2->eo - s2->so) == strlen(s2->sym->name);
> + exact1 = (s1->eo - s1->so) == (off_t)strlen(s1->sym->name);
> + exact2 = (s2->eo - s2->so) == (off_t)strlen(s2->sym->name);
> if (exact1 && !exact2)
> return -1;
> if (!exact1 && exact2)
> @@ -1390,31 +1396,33 @@ const char *prop_get_type_name(enum prop_type type)
> return "unknown";
> }
>
> -static void prop_add_env(const char *env)
> +static void prop_add_env(const char *env_key)
> {
> struct symbol *sym, *sym2;
> struct property *prop;
> - char *p;
> + char *env_val;
>
> sym = current_entry->sym;
> sym->flags |= SYMBOL_AUTO;
> for_all_properties(sym, prop, P_ENV) {
> sym2 = prop_get_symbol(prop);
> - if (strcmp(sym2->name, env))
> + if (strcmp(sym2->name, env_key))
> menu_warn(current_entry, "redefining environment symbol from %s",
> sym2->name);
> return;
> }
>
> prop = prop_alloc(P_ENV, sym);
> - prop->expr = expr_alloc_symbol(sym_lookup(env, SYMBOL_CONST));
> + prop->expr = expr_alloc_symbol(sym_lookup(env_key, SYMBOL_CONST));
>
> sym_env_list = expr_alloc_one(E_LIST, sym_env_list);
> sym_env_list->right.sym = sym;
> + env_val = getenv(env_key);
>
> - p = getenv(env);
> - if (p)
> - sym_add_default(sym, p);
> - else
> - menu_warn(current_entry, "environment variable %s undefined", env);
> + if (likely(env_val)) {
> + sym_add_default(sym, env_val);
> + return;
> + }
> +
> + menu_warn(current_entry, "environment variable %s undefined", env_key);
> }
> --
> 2.16.2
--
Best Regards
Masahiro Yamada
On Mon, Mar 26, 2018 at 01:52:26AM +0900, Masahiro Yamada wrote:
> I want to see Kconfig improvements in a bigger picture.
>
> The changes below are noise.
That's understandable; I do agree that nothing here is _fundamentally_
broken at all, so no worries.
--
Cheers,
Joey Pabalinas