[RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

[RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

[Taeung Song]

Hello, :)

When initializing default perf config values,
we currently use values of actual type(int, bool, char *, etc.).
But I suggest using default config key-value pairs arrays.

For example,
If there isn't user config value at ~/.perfconfig for 'annotate.use_offset' config variable,
default value for it is 'true' bool type value in perf like below.

At ui/browsers/annoate.c

static struct annotate_browser_opt {
       bool hide_src_code,
            use_offset,
	    jump_arrows,
	    show_linenr,
	    show_nr_jumps,
	    show_total_period;
} annotate_browser__opts = {
       .use_offset      = true,
       .jump_arrows     = true,
};

But if we use new config arrays that have all default config key-value pairs,
we could initialize default config values with them.

If we do, we can manage default perf config values at one spot (like util/config.c)
and It can be easy and simple to modify default config values or add new configs.

For example,
If we use new default config arrays and there isn't user config value for 'annoate.use_offset'
default value for it will be set as annotate_config_items[CONFIG_ANNOATE_USE_OFFSET].value
instead of actual boolean type value 'true'.

IMHO, I think it would needed to use new default config arrays
to manage default perf config values more effectively.
And this pathset contains patchs for only 'colors' and 'annoate' section
because waiting for other opinions.

If you review this patchset, I'd appreciate it :-)

Thanks,
Taeung

v5:
- rebased on current acme/perf/core

v4:
- rename 'fb_ground' to 'fore_back_colors' (Namhyung)
- add struct default_config_section
- split first patch[PATCH 1/7] as two
- remove perf_default_config_init() at perf.c
- rebased on current acme/perf/core

v3:
- remove default config arrays for the rest sections except 'colors' and 'annotate'
- use combined {fore, back}ground colors instead of each two color
- introduce perf_default_config_init() that call all default_*_config_init()
  for each config section

v2:
- rename 'ui_browser__config_gcolors' to 'ui_browser__config_colors' (Arnaldo)
- change 'ground colors' to '{back, fore}ground colors' (Arnaldo)
- use strtok + ltrim instead of strchr and while (isspace(*++bg)); (Arnaldo)

Taeung Song (7):
  perf config: Introduce default_config_section and default_config_item
    for default config key-value pairs
  perf config: Add macros assigning key-value pairs to
    default_config_item
  perf config: Add 'colors' section default configs arrrays
  perf config: Use combined {fore,back}ground colors value instead of
    each two color
  perf config: Initialize ui_browser__colorsets with default config
    items
  perf config: Add 'annotate' section default configs arrrays
  perf config: Initialize annotate_browser__opts with default config
    items

 tools/perf/ui/browser.c           | 64 +++++++++++++++++-------------
 tools/perf/ui/browsers/annotate.c | 16 ++++++--
 tools/perf/util/config.c          | 26 +++++++++++++
 tools/perf/util/config.h          | 82 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+), 32 deletions(-)

-- 
2.5.0

[PATCH v5 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs

[Taeung Song]

When initializing default perf config values,
we currently use values of actual type(int, bool, char *, etc.).

For example,
If there isn't user config value at ~/.perfconfig
for 'annotate.use_offset' config variable,
default value for it is 'true' bool type value in perf like below.

At ui/browsers/annoate.c

static struct annotate_browser_opt {
       bool hide_src_code,
            use_offset,
	    jump_arrows,
	    show_linenr,
	    show_nr_jumps,
	    show_total_period;
} annotate_browser__opts = {
       .use_offset      = true,
       .jump_arrows     = true,
};

But I suggest using 'struct default_config_section' and
'struct default_config_item' that have default config key-value pairs
in order to initialize default config values with them, in near future.

If we do, we could manage default perf config values at one spot (i.e. util/config.c)
with default config arrays and it could be easy and simple to modify
default config values or add new configs.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 6f813d4..1fd8e4c 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -63,4 +63,33 @@ void perf_config__refresh(void);
 	perf_config_sections__for_each_entry(&set->sections, section)		\
 	perf_config_items__for_each_entry(&section->items, item)
 
+enum perf_config_type {
+	CONFIG_TYPE_BOOL,
+	CONFIG_TYPE_INT,
+	CONFIG_TYPE_LONG,
+	CONFIG_TYPE_U64,
+	CONFIG_TYPE_FLOAT,
+	CONFIG_TYPE_DOUBLE,
+	CONFIG_TYPE_STRING
+};
+
+struct default_config_item {
+	const char *name;
+	union {
+		bool b;
+		int i;
+		u32 l;
+		u64 ll;
+		float f;
+		double d;
+		const char *s;
+	} value;
+	enum perf_config_type type;
+};
+
+struct default_config_section {
+	const char *name;
+	const struct default_config_item *items;
+};
+
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

[PATCH v5 2/7] perf config: Add macros assigning key-value pairs to default_config_item

[Taeung Song]

In near future, default_config_item arrays will be added
(e.g. const struct default_config_item colors_config_items[])
To simply assign config key-value pairs to default_config_item,
add macros that are CONF_VAR() and CONF_*_VAR() for each type.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 1fd8e4c..613900f 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -92,4 +92,24 @@ struct default_config_section {
 	const struct default_config_item *items;
 };
 
+#define CONF_VAR(_name, _field, _val, _type)			\
+	{ .name = _name, .value._field = _val, .type = _type }
+
+#define CONF_BOOL_VAR(_name, _val)			\
+	CONF_VAR(_name, b, _val, CONFIG_TYPE_BOOL)
+#define CONF_INT_VAR(_name, _val)			\
+	CONF_VAR(_name, i, _val, CONFIG_TYPE_INT)
+#define CONF_LONG_VAR(_name, _val)			\
+	CONF_VAR(_name, l, _val, CONFIG_TYPE_LONG)
+#define CONF_U64_VAR(_name, _val)			\
+	CONF_VAR(_name, ll, _val, CONFIG_TYPE_U64)
+#define CONF_FLOAT_VAR(_name, _val)			\
+	CONF_VAR(_name, f, _val, CONFIG_TYPE_FLOAT)
+#define CONF_DOUBLE_VAR(_name, _val)			\
+	CONF_VAR(_name, d, _val, CONFIG_TYPE_DOUBLE)
+#define CONF_STR_VAR(_name, _val)			\
+	CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
+#define CONF_END()					\
+	{ .name = NULL }
+
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

[PATCH v5 3/7] perf config: Add 'colors' section default configs arrrays

[Taeung Song]

Actual variable for configs of 'colors' section is like below.

(at ui/browser.c)
static struct ui_browser_colorset {
	const char *name, *fg, *bg;
	int colorset;
} ui_browser__colorsets[] = {
	{
		.colorset = HE_COLORSET_TOP,
		.name	  = "top",
		.fg	  = "red",
		.bg	  = "default",
	},
...

But I suggest using 'colors' default config array that
have all default config key-value pairs for 'colors' section

In near future, this array will be used on ui/browser.c
because of setting default actual variable for 'colors' config.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.c | 15 +++++++++++++++
 tools/perf/util/config.h | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 18dae74..a0c0170 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -30,6 +30,21 @@ static struct perf_config_set *config_set;
 
 const char *config_exclusive_filename;
 
+const struct default_config_item colors_config_items[] = {
+	CONF_STR_VAR("top", "red, default"),
+	CONF_STR_VAR("medium", "green, default"),
+	CONF_STR_VAR("normal", "default, default"),
+	CONF_STR_VAR("selected", "black, yellow"),
+	CONF_STR_VAR("jump_arrows", "blue, default"),
+	CONF_STR_VAR("addr", "magenta, default"),
+	CONF_STR_VAR("root", "white, blue"),
+	CONF_END()
+};
+
+const struct default_config_section default_sections[] = {
+	{ .name = "colors", .items = colors_config_items },
+};
+
 static int get_next_char(void)
 {
 	int c;
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 613900f..f2ce8b4 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -73,6 +73,20 @@ enum perf_config_type {
 	CONFIG_TYPE_STRING
 };
 
+enum config_section_idx {
+	CONFIG_COLORS,
+};
+
+enum colors_config_items_idx {
+	CONFIG_COLORS_TOP,
+	CONFIG_COLORS_MEDIUM,
+	CONFIG_COLORS_NORMAL,
+	CONFIG_COLORS_SELECTED,
+	CONFIG_COLORS_JUMP_ARROWS,
+	CONFIG_COLORS_ADDR,
+	CONFIG_COLORS_ROOT,
+};
+
 struct default_config_item {
 	const char *name;
 	union {
@@ -112,4 +126,6 @@ struct default_config_section {
 #define CONF_END()					\
 	{ .name = NULL }
 
+extern const struct default_config_item colors_config_items[];
+
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

[PATCH v5 4/7] perf config: Use combined {fore,back}ground colors value instead of each two color

[Taeung Song]

To manage all default config values at one spot (at util/config.c),
it would be better that actual variables for each 'colors' config
also have only one value like 'default_config_item' type.

If we do, it smoothly work to initialize 'colors' default config values
by 'colors_config_items' array that have default values at util/config.c
because both actual variable and config item of 'colors_config_items'
are equal in the number of values (as just one).

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/ui/browser.c | 81 ++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 42 deletions(-)

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 3eb3edb..b4e21d1 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -503,61 +503,53 @@ unsigned int ui_browser__list_head_refresh(struct ui_browser *browser)
 }
 
 static struct ui_browser_colorset {
-	const char *name, *fg, *bg;
+	const char *name, *fore_back_colors;
 	int colorset;
 } ui_browser__colorsets[] = {
 	{
-		.colorset = HE_COLORSET_TOP,
-		.name	  = "top",
-		.fg	  = "red",
-		.bg	  = "default",
+		.colorset  = HE_COLORSET_TOP,
+		.name      = "top",
+		.fore_back_colors = "red, default",
 	},
 	{
-		.colorset = HE_COLORSET_MEDIUM,
-		.name	  = "medium",
-		.fg	  = "green",
-		.bg	  = "default",
+		.colorset  = HE_COLORSET_MEDIUM,
+		.name      = "medium",
+		.fore_back_colors = "green, default",
 	},
 	{
-		.colorset = HE_COLORSET_NORMAL,
-		.name	  = "normal",
-		.fg	  = "default",
-		.bg	  = "default",
+		.colorset  = HE_COLORSET_NORMAL,
+		.name      = "normal",
+		.fore_back_colors = "default, default",
 	},
 	{
-		.colorset = HE_COLORSET_SELECTED,
-		.name	  = "selected",
-		.fg	  = "black",
-		.bg	  = "yellow",
+		.colorset  = HE_COLORSET_SELECTED,
+		.name      = "selected",
+		.fore_back_colors = "black, yellow",
 	},
 	{
-		.colorset = HE_COLORSET_JUMP_ARROWS,
-		.name	  = "jump_arrows",
-		.fg	  = "blue",
-		.bg	  = "default",
+		.colorset  = HE_COLORSET_JUMP_ARROWS,
+		.name      = "jump_arrows",
+		.fore_back_colors = "blue, default",
 	},
 	{
-		.colorset = HE_COLORSET_ADDR,
-		.name	  = "addr",
-		.fg	  = "magenta",
-		.bg	  = "default",
+		.colorset  = HE_COLORSET_ADDR,
+		.name      = "addr",
+		.fore_back_colors = "magenta, default",
 	},
 	{
-		.colorset = HE_COLORSET_ROOT,
-		.name	  = "root",
-		.fg	  = "white",
-		.bg	  = "blue",
+		.colorset  = HE_COLORSET_ROOT,
+		.name      = "root",
+		.fore_back_colors = "white, blue",
 	},
 	{
 		.name = NULL,
 	}
 };
 
-
 static int ui_browser__color_config(const char *var, const char *value,
 				    void *data __maybe_unused)
 {
-	char *fg = NULL, *bg;
+	char *fore_back_colors;
 	int i;
 
 	/* same dir for all commands */
@@ -570,22 +562,18 @@ static int ui_browser__color_config(const char *var, const char *value,
 		if (strcmp(ui_browser__colorsets[i].name, name) != 0)
 			continue;
 
-		fg = strdup(value);
-		if (fg == NULL)
-			break;
+		if (strstr(value, ",") == NULL)
+			return -1;
 
-		bg = strchr(fg, ',');
-		if (bg == NULL)
+		fore_back_colors = strdup(value);
+		if (fore_back_colors == NULL)
 			break;
+		ui_browser__colorsets[i].fore_back_colors = fore_back_colors;
 
-		*bg = '\0';
-		while (isspace(*++bg));
-		ui_browser__colorsets[i].bg = bg;
-		ui_browser__colorsets[i].fg = fg;
 		return 0;
 	}
 
-	free(fg);
+	free(fore_back_colors);
 	return -1;
 }
 
@@ -743,8 +731,17 @@ void ui_browser__init(void)
 	perf_config(ui_browser__color_config, NULL);
 
 	while (ui_browser__colorsets[i].name) {
+		char *fore_back_colors, *fg, *bg;
 		struct ui_browser_colorset *c = &ui_browser__colorsets[i++];
-		sltt_set_color(c->colorset, c->name, c->fg, c->bg);
+
+		fore_back_colors = strdup(c->fore_back_colors);
+		if (fg == NULL)
+			break;
+		fg = strtok(fore_back_colors, ",");
+		bg = strtok(NULL, ",");
+		bg = ltrim(bg);
+		sltt_set_color(c->colorset, c->name, fg, bg);
+		free(fore_back_colors);
 	}
 
 	annotate_browser__init();
-- 
2.5.0

[PATCH v5 5/7] perf config: Initialize ui_browser__colorsets with default config items

[Taeung Song]

Set default config values for 'colors' section with 'colors_config_items[]'
instead of actual const char * type values.
(e.g. using colors_config_item[CONFIG_COLORS_TOP].value
instead of "red, default" string value for 'colors.top')

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/ui/browser.c | 53 +++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index b4e21d1..380abab 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -507,39 +507,32 @@ static struct ui_browser_colorset {
 	int colorset;
 } ui_browser__colorsets[] = {
 	{
-		.colorset  = HE_COLORSET_TOP,
-		.name      = "top",
-		.fore_back_colors = "red, default",
+		.colorset = HE_COLORSET_TOP,
+		.name	  = "top",
 	},
 	{
-		.colorset  = HE_COLORSET_MEDIUM,
-		.name      = "medium",
-		.fore_back_colors = "green, default",
+		.colorset = HE_COLORSET_MEDIUM,
+		.name	  = "medium",
 	},
 	{
-		.colorset  = HE_COLORSET_NORMAL,
-		.name      = "normal",
-		.fore_back_colors = "default, default",
+		.colorset = HE_COLORSET_NORMAL,
+		.name	  = "normal",
 	},
 	{
-		.colorset  = HE_COLORSET_SELECTED,
-		.name      = "selected",
-		.fore_back_colors = "black, yellow",
+		.colorset = HE_COLORSET_SELECTED,
+		.name	  = "selected",
 	},
 	{
-		.colorset  = HE_COLORSET_JUMP_ARROWS,
-		.name      = "jump_arrows",
-		.fore_back_colors = "blue, default",
+		.colorset = HE_COLORSET_JUMP_ARROWS,
+		.name	  = "jump_arrows",
 	},
 	{
-		.colorset  = HE_COLORSET_ADDR,
-		.name      = "addr",
-		.fore_back_colors = "magenta, default",
+		.colorset = HE_COLORSET_ADDR,
+		.name	  = "addr",
 	},
 	{
-		.colorset  = HE_COLORSET_ROOT,
-		.name      = "root",
-		.fore_back_colors = "white, blue",
+		.colorset = HE_COLORSET_ROOT,
+		.name	  = "root",
 	},
 	{
 		.name = NULL,
@@ -724,10 +717,28 @@ void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
 		__ui_browser__line_arrow_down(browser, column, start, end);
 }
 
+static void default_colors_config_init(void)
+{
+	int i, j;
+
+	for (i = 0; ui_browser__colorsets[i].name != NULL; ++i) {
+		const char *name = ui_browser__colorsets[i].name;
+
+		for (j = 0; colors_config_items[j].name != NULL; j++) {
+			if (!strcmp(name, colors_config_items[j].name)) {
+				ui_browser__colorsets[i].fore_back_colors =
+					colors_config_items[j].value.s;
+				break;
+			}
+		}
+	}
+}
+
 void ui_browser__init(void)
 {
 	int i = 0;
 
+	default_colors_config_init();
 	perf_config(ui_browser__color_config, NULL);
 
 	while (ui_browser__colorsets[i].name) {
-- 
2.5.0

Re: [PATCH v5 5/7] perf config: Initialize ui_browser__colorsets with default config items

[Namhyung Kim]

On Wed, Jul 06, 2016 at 02:20:21PM +0900, Taeung Song wrote:
> Set default config values for 'colors' section with 'colors_config_items[]'
> instead of actual const char * type values.
> (e.g. using colors_config_item[CONFIG_COLORS_TOP].value
> instead of "red, default" string value for 'colors.top')
> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/ui/browser.c | 53 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index b4e21d1..380abab 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -507,39 +507,32 @@ static struct ui_browser_colorset {
>  	int colorset;
>  } ui_browser__colorsets[] = {
>  	{
> -		.colorset  = HE_COLORSET_TOP,
> -		.name      = "top",
> -		.fore_back_colors = "red, default",
> +		.colorset = HE_COLORSET_TOP,
> +		.name	  = "top",

It seems like an unnecessary whitespace change, please fix the patch 4.

Thanks,
Namhyung


>  	},
>  	{
> -		.colorset  = HE_COLORSET_MEDIUM,
> -		.name      = "medium",
> -		.fore_back_colors = "green, default",
> +		.colorset = HE_COLORSET_MEDIUM,
> +		.name	  = "medium",
>  	},
>  	{
> -		.colorset  = HE_COLORSET_NORMAL,
> -		.name      = "normal",
> -		.fore_back_colors = "default, default",
> +		.colorset = HE_COLORSET_NORMAL,
> +		.name	  = "normal",
>  	},
>  	{
> -		.colorset  = HE_COLORSET_SELECTED,
> -		.name      = "selected",
> -		.fore_back_colors = "black, yellow",
> +		.colorset = HE_COLORSET_SELECTED,
> +		.name	  = "selected",
>  	},
>  	{
> -		.colorset  = HE_COLORSET_JUMP_ARROWS,
> -		.name      = "jump_arrows",
> -		.fore_back_colors = "blue, default",
> +		.colorset = HE_COLORSET_JUMP_ARROWS,
> +		.name	  = "jump_arrows",
>  	},
>  	{
> -		.colorset  = HE_COLORSET_ADDR,
> -		.name      = "addr",
> -		.fore_back_colors = "magenta, default",
> +		.colorset = HE_COLORSET_ADDR,
> +		.name	  = "addr",
>  	},
>  	{
> -		.colorset  = HE_COLORSET_ROOT,
> -		.name      = "root",
> -		.fore_back_colors = "white, blue",
> +		.colorset = HE_COLORSET_ROOT,
> +		.name	  = "root",
>  	},
>  	{
>  		.name = NULL,
> @@ -724,10 +717,28 @@ void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
>  		__ui_browser__line_arrow_down(browser, column, start, end);
>  }
>  
> +static void default_colors_config_init(void)
> +{
> +	int i, j;
> +
> +	for (i = 0; ui_browser__colorsets[i].name != NULL; ++i) {
> +		const char *name = ui_browser__colorsets[i].name;
> +
> +		for (j = 0; colors_config_items[j].name != NULL; j++) {
> +			if (!strcmp(name, colors_config_items[j].name)) {
> +				ui_browser__colorsets[i].fore_back_colors =
> +					colors_config_items[j].value.s;
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  void ui_browser__init(void)
>  {
>  	int i = 0;
>  
> +	default_colors_config_init();
>  	perf_config(ui_browser__color_config, NULL);
>  
>  	while (ui_browser__colorsets[i].name) {
> -- 
> 2.5.0
> 

Re: [PATCH v5 5/7] perf config: Initialize ui_browser__colorsets with default config items

[Taeung Song]

On 07/12/2016 02:39 PM, Namhyung Kim wrote:
> On Wed, Jul 06, 2016 at 02:20:21PM +0900, Taeung Song wrote:
>> Set default config values for 'colors' section with 'colors_config_items[]'
>> instead of actual const char * type values.
>> (e.g. using colors_config_item[CONFIG_COLORS_TOP].value
>> instead of "red, default" string value for 'colors.top')
>>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Wang Nan <wangnan0@huawei.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/ui/browser.c | 53 +++++++++++++++++++++++++++++--------------------
>>   1 file changed, 32 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>> index b4e21d1..380abab 100644
>> --- a/tools/perf/ui/browser.c
>> +++ b/tools/perf/ui/browser.c
>> @@ -507,39 +507,32 @@ static struct ui_browser_colorset {
>>   	int colorset;
>>   } ui_browser__colorsets[] = {
>>   	{
>> -		.colorset  = HE_COLORSET_TOP,
>> -		.name      = "top",
>> -		.fore_back_colors = "red, default",
>> +		.colorset = HE_COLORSET_TOP,
>> +		.name	  = "top",
>
> It seems like an unnecessary whitespace change, please fix the patch 4.
>

OK, renaming 'fore_back_colors' to simple 'colors' as you said,
I'll fix the patch 4 !

Thanks,
Taeung

>
>
>>   	},
>>   	{
>> -		.colorset  = HE_COLORSET_MEDIUM,
>> -		.name      = "medium",
>> -		.fore_back_colors = "green, default",
>> +		.colorset = HE_COLORSET_MEDIUM,
>> +		.name	  = "medium",
>>   	},
>>   	{
>> -		.colorset  = HE_COLORSET_NORMAL,
>> -		.name      = "normal",
>> -		.fore_back_colors = "default, default",
>> +		.colorset = HE_COLORSET_NORMAL,
>> +		.name	  = "normal",
>>   	},
>>   	{
>> -		.colorset  = HE_COLORSET_SELECTED,
>> -		.name      = "selected",
>> -		.fore_back_colors = "black, yellow",
>> +		.colorset = HE_COLORSET_SELECTED,
>> +		.name	  = "selected",
>>   	},
>>   	{
>> -		.colorset  = HE_COLORSET_JUMP_ARROWS,
>> -		.name      = "jump_arrows",
>> -		.fore_back_colors = "blue, default",
>> +		.colorset = HE_COLORSET_JUMP_ARROWS,
>> +		.name	  = "jump_arrows",
>>   	},
>>   	{
>> -		.colorset  = HE_COLORSET_ADDR,
>> -		.name      = "addr",
>> -		.fore_back_colors = "magenta, default",
>> +		.colorset = HE_COLORSET_ADDR,
>> +		.name	  = "addr",
>>   	},
>>   	{
>> -		.colorset  = HE_COLORSET_ROOT,
>> -		.name      = "root",
>> -		.fore_back_colors = "white, blue",
>> +		.colorset = HE_COLORSET_ROOT,
>> +		.name	  = "root",
>>   	},
>>   	{
>>   		.name = NULL,
>> @@ -724,10 +717,28 @@ void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
>>   		__ui_browser__line_arrow_down(browser, column, start, end);
>>   }
>>
>> +static void default_colors_config_init(void)
>> +{
>> +	int i, j;
>> +
>> +	for (i = 0; ui_browser__colorsets[i].name != NULL; ++i) {
>> +		const char *name = ui_browser__colorsets[i].name;
>> +
>> +		for (j = 0; colors_config_items[j].name != NULL; j++) {
>> +			if (!strcmp(name, colors_config_items[j].name)) {
>> +				ui_browser__colorsets[i].fore_back_colors =
>> +					colors_config_items[j].value.s;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +}
>> +
>>   void ui_browser__init(void)
>>   {
>>   	int i = 0;
>>
>> +	default_colors_config_init();
>>   	perf_config(ui_browser__color_config, NULL);
>>
>>   	while (ui_browser__colorsets[i].name) {
>> --
>> 2.5.0
>>

[PATCH v5 6/7] perf config: Add 'annotate' section default configs arrrays

[Taeung Song]

Actual variable for configs of 'annotate' section is like below.

(at ui/browsers/annoate.c)
static struct annotate_browser_opt {
       bool hide_src_code,
            use_offset,
	    jump_arrows,
	    show_linenr,
	    show_nr_jumps,
	    show_total_period;
} annotate_browser__opts = {
       .use_offset      = true,
       .jump_arrows     = true,
};

But I suggest using 'annoate' default config array that
have all default config key-value pairs for 'annotate' section

In near future, this arrays will be used on ui/browsers/annoate.c
because of setting default actual variable for 'annotate' config.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.c | 11 +++++++++++
 tools/perf/util/config.h | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index a0c0170..d8d5415 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -41,8 +41,19 @@ const struct default_config_item colors_config_items[] = {
 	CONF_END()
 };
 
+const struct default_config_item annotate_config_items[] = {
+	CONF_BOOL_VAR("hide_src_code", false),
+	CONF_BOOL_VAR("use_offset", true),
+	CONF_BOOL_VAR("jump_arrows", true),
+	CONF_BOOL_VAR("show_nr_jumps", false),
+	CONF_BOOL_VAR("show_linenr", false),
+	CONF_BOOL_VAR("show_total_period", false),
+	CONF_END()
+};
+
 const struct default_config_section default_sections[] = {
 	{ .name = "colors", .items = colors_config_items },
+	{ .name = "annotate", .items = annotate_config_items },
 };
 
 static int get_next_char(void)
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index f2ce8b4..470c93a 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -75,6 +75,7 @@ enum perf_config_type {
 
 enum config_section_idx {
 	CONFIG_COLORS,
+	CONFIG_ANNOTATE,
 };
 
 enum colors_config_items_idx {
@@ -87,6 +88,15 @@ enum colors_config_items_idx {
 	CONFIG_COLORS_ROOT,
 };
 
+enum annotate_config_items_idx {
+	CONFIG_ANNOTATE_HIDE_SRC_CODE,
+	CONFIG_ANNOTATE_USE_OFFSET,
+	CONFIG_ANNOTATE_JUMP_ARROWS,
+	CONFIG_ANNOTATE_SHOW_NR_JUMPS,
+	CONFIG_ANNOTATE_SHOW_LINENR,
+	CONFIG_ANNOTATE_SHOW_TOTAL_PERIOD,
+};
+
 struct default_config_item {
 	const char *name;
 	union {
@@ -127,5 +137,6 @@ struct default_config_section {
 	{ .name = NULL }
 
 extern const struct default_config_item colors_config_items[];
+extern const struct default_config_item annotate_config_items[];
 
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

[PATCH v5 7/7] perf config: Initialize annotate_browser__opts with default config items

[Taeung Song]

Set default config values for 'annotate' section with 'annotate_config_items[]'
instead of actual bool type values.
(e.g. using annotate_config_items[CONFIG_ANNOTATE_USE_OFFSET].value
instead of 'true' bool type value for 'annotate.use_offset'.)

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/ui/browsers/annotate.c | 16 ++++++++++++----
 tools/perf/util/config.h          |  6 ++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 29dc6d2..0fb78b5 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -38,10 +38,7 @@ static struct annotate_browser_opt {
 	     show_linenr,
 	     show_nr_jumps,
 	     show_total_period;
-} annotate_browser__opts = {
-	.use_offset	= true,
-	.jump_arrows	= true,
-};
+} annotate_browser__opts;
 
 struct annotate_browser {
 	struct ui_browser b;
@@ -1157,7 +1154,18 @@ static int annotate__config(const char *var, const char *value,
 	return 0;
 }
 
+static void default_annotate_config_init(void)
+{
+	annotate_browser__opts.hide_src_code = CONF_ANNOTATE_DEFAULT_VAL(HIDE_SRC_CODE, b);
+	annotate_browser__opts.use_offset = CONF_ANNOTATE_DEFAULT_VAL(USE_OFFSET, b);
+	annotate_browser__opts.jump_arrows = CONF_ANNOTATE_DEFAULT_VAL(JUMP_ARROWS, b);
+	annotate_browser__opts.show_linenr = CONF_ANNOTATE_DEFAULT_VAL(SHOW_LINENR, b);
+	annotate_browser__opts.show_nr_jumps = CONF_ANNOTATE_DEFAULT_VAL(SHOW_NR_JUMPS, b);
+	annotate_browser__opts.show_total_period = CONF_ANNOTATE_DEFAULT_VAL(SHOW_TOTAL_PERIOD, b);
+}
+
 void annotate_browser__init(void)
 {
+	default_annotate_config_init();
 	perf_config(annotate__config, NULL);
 }
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 470c93a..c30e6bb 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -136,6 +136,12 @@ struct default_config_section {
 #define CONF_END()					\
 	{ .name = NULL }
 
+#define CONF_DEFAULT_VAL(section, name, field)			\
+	section##_config_items[CONFIG_##name].value.field
+
+#define CONF_ANNOTATE_DEFAULT_VAL(name, field)			\
+	CONF_DEFAULT_VAL(annotate, ANNOTATE_##name, field)
+
 extern const struct default_config_item colors_config_items[];
 extern const struct default_config_item annotate_config_items[];
 
-- 
2.5.0

Re: [PATCH v5 7/7] perf config: Initialize annotate_browser__opts with default config items

[Namhyung Kim]

On Wed, Jul 06, 2016 at 02:20:23PM +0900, Taeung Song wrote:
> Set default config values for 'annotate' section with 'annotate_config_items[]'
> instead of actual bool type values.
> (e.g. using annotate_config_items[CONFIG_ANNOTATE_USE_OFFSET].value
> instead of 'true' bool type value for 'annotate.use_offset'.)
> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/ui/browsers/annotate.c | 16 ++++++++++++----
>  tools/perf/util/config.h          |  6 ++++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 29dc6d2..0fb78b5 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -38,10 +38,7 @@ static struct annotate_browser_opt {
>  	     show_linenr,
>  	     show_nr_jumps,
>  	     show_total_period;
> -} annotate_browser__opts = {
> -	.use_offset	= true,
> -	.jump_arrows	= true,
> -};
> +} annotate_browser__opts;
>  
>  struct annotate_browser {
>  	struct ui_browser b;
> @@ -1157,7 +1154,18 @@ static int annotate__config(const char *var, const char *value,
>  	return 0;
>  }
>  
> +static void default_annotate_config_init(void)
> +{
> +	annotate_browser__opts.hide_src_code = CONF_ANNOTATE_DEFAULT_VAL(HIDE_SRC_CODE, b);
> +	annotate_browser__opts.use_offset = CONF_ANNOTATE_DEFAULT_VAL(USE_OFFSET, b);
> +	annotate_browser__opts.jump_arrows = CONF_ANNOTATE_DEFAULT_VAL(JUMP_ARROWS, b);
> +	annotate_browser__opts.show_linenr = CONF_ANNOTATE_DEFAULT_VAL(SHOW_LINENR, b);
> +	annotate_browser__opts.show_nr_jumps = CONF_ANNOTATE_DEFAULT_VAL(SHOW_NR_JUMPS, b);
> +	annotate_browser__opts.show_total_period = CONF_ANNOTATE_DEFAULT_VAL(SHOW_TOTAL_PERIOD, b);
> +}
> +
>  void annotate_browser__init(void)
>  {
> +	default_annotate_config_init();
>  	perf_config(annotate__config, NULL);
>  }
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index 470c93a..c30e6bb 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -136,6 +136,12 @@ struct default_config_section {
>  #define CONF_END()					\
>  	{ .name = NULL }
>  
> +#define CONF_DEFAULT_VAL(section, name, field)			\
> +	section##_config_items[CONFIG_##name].value.field
> +
> +#define CONF_ANNOTATE_DEFAULT_VAL(name, field)			\
> +	CONF_DEFAULT_VAL(annotate, ANNOTATE_##name, field)
> +

Instead of making accessor macro for each config section, can we make
it more general like below?

  #define CONF_DEFAULT_BOOL(sec, name) \
    default_sections[sec##_IDX].items[sec##_##name].b

  opts->hide_src_code = CONF_DEFAULT_BOOL(ANNOTATE, HIDE_SRC_CODE);


Thanks,
Namhyung


>  extern const struct default_config_item colors_config_items[];
>  extern const struct default_config_item annotate_config_items[];
>  
> -- 
> 2.5.0
> 

Re: [PATCH v5 7/7] perf config: Initialize annotate_browser__opts with default config items

[Taeung Song]

On 07/12/2016 02:47 PM, Namhyung Kim wrote:
> On Wed, Jul 06, 2016 at 02:20:23PM +0900, Taeung Song wrote:
>> Set default config values for 'annotate' section with 'annotate_config_items[]'
>> instead of actual bool type values.
>> (e.g. using annotate_config_items[CONFIG_ANNOTATE_USE_OFFSET].value
>> instead of 'true' bool type value for 'annotate.use_offset'.)
>>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Wang Nan <wangnan0@huawei.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/ui/browsers/annotate.c | 16 ++++++++++++----
>>   tools/perf/util/config.h          |  6 ++++++
>>   2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>> index 29dc6d2..0fb78b5 100644
>> --- a/tools/perf/ui/browsers/annotate.c
>> +++ b/tools/perf/ui/browsers/annotate.c
>> @@ -38,10 +38,7 @@ static struct annotate_browser_opt {
>>   	     show_linenr,
>>   	     show_nr_jumps,
>>   	     show_total_period;
>> -} annotate_browser__opts = {
>> -	.use_offset	= true,
>> -	.jump_arrows	= true,
>> -};
>> +} annotate_browser__opts;
>>
>>   struct annotate_browser {
>>   	struct ui_browser b;
>> @@ -1157,7 +1154,18 @@ static int annotate__config(const char *var, const char *value,
>>   	return 0;
>>   }
>>
>> +static void default_annotate_config_init(void)
>> +{
>> +	annotate_browser__opts.hide_src_code = CONF_ANNOTATE_DEFAULT_VAL(HIDE_SRC_CODE, b);
>> +	annotate_browser__opts.use_offset = CONF_ANNOTATE_DEFAULT_VAL(USE_OFFSET, b);
>> +	annotate_browser__opts.jump_arrows = CONF_ANNOTATE_DEFAULT_VAL(JUMP_ARROWS, b);
>> +	annotate_browser__opts.show_linenr = CONF_ANNOTATE_DEFAULT_VAL(SHOW_LINENR, b);
>> +	annotate_browser__opts.show_nr_jumps = CONF_ANNOTATE_DEFAULT_VAL(SHOW_NR_JUMPS, b);
>> +	annotate_browser__opts.show_total_period = CONF_ANNOTATE_DEFAULT_VAL(SHOW_TOTAL_PERIOD, b);
>> +}
>> +
>>   void annotate_browser__init(void)
>>   {
>> +	default_annotate_config_init();
>>   	perf_config(annotate__config, NULL);
>>   }
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index 470c93a..c30e6bb 100644
>> --- a/tools/perf/util/config.h
>> +++ b/tools/perf/util/config.h
>> @@ -136,6 +136,12 @@ struct default_config_section {
>>   #define CONF_END()					\
>>   	{ .name = NULL }
>>
>> +#define CONF_DEFAULT_VAL(section, name, field)			\
>> +	section##_config_items[CONFIG_##name].value.field
>> +
>> +#define CONF_ANNOTATE_DEFAULT_VAL(name, field)			\
>> +	CONF_DEFAULT_VAL(annotate, ANNOTATE_##name, field)
>> +
>
> Instead of making accessor macro for each config section, can we make
> it more general like below?
>
>    #define CONF_DEFAULT_BOOL(sec, name) \
>      default_sections[sec##_IDX].items[sec##_##name].b
>
>    opts->hide_src_code = CONF_DEFAULT_BOOL(ANNOTATE, HIDE_SRC_CODE);
>
>

Understood!
I'll change the macro to general macro as you said. :-D

Thanks,
Taeung

>
>
>>   extern const struct default_config_item colors_config_items[];
>>   extern const struct default_config_item annotate_config_items[];
>>
>> --
>> 2.5.0
>>

Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

[Taeung Song]

Hi, Namhyung :)

As you know, the patch work from v3 to v4 took too long time
because of a patchset refactoring perf_config().

So I guess it is hard for you to recall this patchset for new default
config arrays but could you a bit response two simple questions ?


1) I renamed 'fb_ground' to 'fore_back_colors' in struct 
ui_browser_colorset at ui/browser.c.

Is it better than before ?

2) I added not only 'struct default_config_item' but also
'struct default_config_section'.
(In order to use const type and 'const struct default_config_item *items'
instead of using 'struct list_head items')

could you agree this way ?


Thanks,
Taeung

On 07/06/2016 02:20 PM, Taeung Song wrote:
> Hello, :)
>
> When initializing default perf config values,
> we currently use values of actual type(int, bool, char *, etc.).
> But I suggest using default config key-value pairs arrays.
>
> For example,
> If there isn't user config value at ~/.perfconfig for 'annotate.use_offset' config variable,
> default value for it is 'true' bool type value in perf like below.
>
> At ui/browsers/annoate.c
>
> static struct annotate_browser_opt {
>         bool hide_src_code,
>              use_offset,
> 	    jump_arrows,
> 	    show_linenr,
> 	    show_nr_jumps,
> 	    show_total_period;
> } annotate_browser__opts = {
>         .use_offset      = true,
>         .jump_arrows     = true,
> };
>
> But if we use new config arrays that have all default config key-value pairs,
> we could initialize default config values with them.
>
> If we do, we can manage default perf config values at one spot (like util/config.c)
> and It can be easy and simple to modify default config values or add new configs.
>
> For example,
> If we use new default config arrays and there isn't user config value for 'annoate.use_offset'
> default value for it will be set as annotate_config_items[CONFIG_ANNOATE_USE_OFFSET].value
> instead of actual boolean type value 'true'.
>
> IMHO, I think it would needed to use new default config arrays
> to manage default perf config values more effectively.
> And this pathset contains patchs for only 'colors' and 'annoate' section
> because waiting for other opinions.
>
> If you review this patchset, I'd appreciate it :-)
>
> Thanks,
> Taeung
>
> v5:
> - rebased on current acme/perf/core
>
> v4:
> - rename 'fb_ground' to 'fore_back_colors' (Namhyung)
> - add struct default_config_section
> - split first patch[PATCH 1/7] as two
> - remove perf_default_config_init() at perf.c
> - rebased on current acme/perf/core
>
> v3:
> - remove default config arrays for the rest sections except 'colors' and 'annotate'
> - use combined {fore, back}ground colors instead of each two color
> - introduce perf_default_config_init() that call all default_*_config_init()
>    for each config section
>
> v2:
> - rename 'ui_browser__config_gcolors' to 'ui_browser__config_colors' (Arnaldo)
> - change 'ground colors' to '{back, fore}ground colors' (Arnaldo)
> - use strtok + ltrim instead of strchr and while (isspace(*++bg)); (Arnaldo)
>
> Taeung Song (7):
>    perf config: Introduce default_config_section and default_config_item
>      for default config key-value pairs
>    perf config: Add macros assigning key-value pairs to
>      default_config_item
>    perf config: Add 'colors' section default configs arrrays
>    perf config: Use combined {fore,back}ground colors value instead of
>      each two color
>    perf config: Initialize ui_browser__colorsets with default config
>      items
>    perf config: Add 'annotate' section default configs arrrays
>    perf config: Initialize annotate_browser__opts with default config
>      items
>
>   tools/perf/ui/browser.c           | 64 +++++++++++++++++-------------
>   tools/perf/ui/browsers/annotate.c | 16 ++++++--
>   tools/perf/util/config.c          | 26 +++++++++++++
>   tools/perf/util/config.h          | 82 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 156 insertions(+), 32 deletions(-)
>

Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

[Namhyung Kim]

Hi Taeung,

On Tue, Jul 12, 2016 at 01:20:35PM +0900, Taeung Song wrote:
> Hi, Namhyung :)
> 
> As you know, the patch work from v3 to v4 took too long time
> because of a patchset refactoring perf_config().
> 
> So I guess it is hard for you to recall this patchset for new default
> config arrays but could you a bit response two simple questions ?
> 
> 
> 1) I renamed 'fb_ground' to 'fore_back_colors' in struct ui_browser_colorset
> at ui/browser.c.
> 
> Is it better than before ?

Not sure.  I don't think the renaming is necessary.  If you do, I
prefer simpler name like 'colors' or 'color_pair'.


> 
> 2) I added not only 'struct default_config_item' but also
> 'struct default_config_section'.
> (In order to use const type and 'const struct default_config_item *items'
> instead of using 'struct list_head items')
> 
> could you agree this way ?

I'm ok with it.

Thanks,
Namhyung


> 
> On 07/06/2016 02:20 PM, Taeung Song wrote:
> > Hello, :)
> > 
> > When initializing default perf config values,
> > we currently use values of actual type(int, bool, char *, etc.).
> > But I suggest using default config key-value pairs arrays.
> > 
> > For example,
> > If there isn't user config value at ~/.perfconfig for 'annotate.use_offset' config variable,
> > default value for it is 'true' bool type value in perf like below.
> > 
> > At ui/browsers/annoate.c
> > 
> > static struct annotate_browser_opt {
> >         bool hide_src_code,
> >              use_offset,
> > 	    jump_arrows,
> > 	    show_linenr,
> > 	    show_nr_jumps,
> > 	    show_total_period;
> > } annotate_browser__opts = {
> >         .use_offset      = true,
> >         .jump_arrows     = true,
> > };
> > 
> > But if we use new config arrays that have all default config key-value pairs,
> > we could initialize default config values with them.
> > 
> > If we do, we can manage default perf config values at one spot (like util/config.c)
> > and It can be easy and simple to modify default config values or add new configs.
> > 
> > For example,
> > If we use new default config arrays and there isn't user config value for 'annoate.use_offset'
> > default value for it will be set as annotate_config_items[CONFIG_ANNOATE_USE_OFFSET].value
> > instead of actual boolean type value 'true'.
> > 
> > IMHO, I think it would needed to use new default config arrays
> > to manage default perf config values more effectively.
> > And this pathset contains patchs for only 'colors' and 'annoate' section
> > because waiting for other opinions.
> > 
> > If you review this patchset, I'd appreciate it :-)
> > 
> > Thanks,
> > Taeung
> > 
> > v5:
> > - rebased on current acme/perf/core
> > 
> > v4:
> > - rename 'fb_ground' to 'fore_back_colors' (Namhyung)
> > - add struct default_config_section
> > - split first patch[PATCH 1/7] as two
> > - remove perf_default_config_init() at perf.c
> > - rebased on current acme/perf/core
> > 
> > v3:
> > - remove default config arrays for the rest sections except 'colors' and 'annotate'
> > - use combined {fore, back}ground colors instead of each two color
> > - introduce perf_default_config_init() that call all default_*_config_init()
> >    for each config section
> > 
> > v2:
> > - rename 'ui_browser__config_gcolors' to 'ui_browser__config_colors' (Arnaldo)
> > - change 'ground colors' to '{back, fore}ground colors' (Arnaldo)
> > - use strtok + ltrim instead of strchr and while (isspace(*++bg)); (Arnaldo)
> > 
> > Taeung Song (7):
> >    perf config: Introduce default_config_section and default_config_item
> >      for default config key-value pairs
> >    perf config: Add macros assigning key-value pairs to
> >      default_config_item
> >    perf config: Add 'colors' section default configs arrrays
> >    perf config: Use combined {fore,back}ground colors value instead of
> >      each two color
> >    perf config: Initialize ui_browser__colorsets with default config
> >      items
> >    perf config: Add 'annotate' section default configs arrrays
> >    perf config: Initialize annotate_browser__opts with default config
> >      items
> > 
> >   tools/perf/ui/browser.c           | 64 +++++++++++++++++-------------
> >   tools/perf/ui/browsers/annotate.c | 16 ++++++--
> >   tools/perf/util/config.c          | 26 +++++++++++++
> >   tools/perf/util/config.h          | 82 +++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 156 insertions(+), 32 deletions(-)
> > 

Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

[Taeung Song]

On 07/12/2016 02:37 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Tue, Jul 12, 2016 at 01:20:35PM +0900, Taeung Song wrote:
>> Hi, Namhyung :)
>>
>> As you know, the patch work from v3 to v4 took too long time
>> because of a patchset refactoring perf_config().
>>
>> So I guess it is hard for you to recall this patchset for new default
>> config arrays but could you a bit response two simple questions ?
>>
>>
>> 1) I renamed 'fb_ground' to 'fore_back_colors' in struct ui_browser_colorset
>> at ui/browser.c.
>>
>> Is it better than before ?
>
> Not sure.  I don't think the renaming is necessary.  If you do, I
> prefer simpler name like 'colors' or 'color_pair'.
>

I got it!

>>
>> 2) I added not only 'struct default_config_item' but also
>> 'struct default_config_section'.
>> (In order to use const type and 'const struct default_config_item *items'
>> instead of using 'struct list_head items')
>>
>> could you agree this way ?
>
> I'm ok with it.
>

Thanks,
Taeung

>
>
>>
>> On 07/06/2016 02:20 PM, Taeung Song wrote:
>>> Hello, :)
>>>
>>> When initializing default perf config values,
>>> we currently use values of actual type(int, bool, char *, etc.).
>>> But I suggest using default config key-value pairs arrays.
>>>
>>> For example,
>>> If there isn't user config value at ~/.perfconfig for 'annotate.use_offset' config variable,
>>> default value for it is 'true' bool type value in perf like below.
>>>
>>> At ui/browsers/annoate.c
>>>
>>> static struct annotate_browser_opt {
>>>          bool hide_src_code,
>>>               use_offset,
>>> 	    jump_arrows,
>>> 	    show_linenr,
>>> 	    show_nr_jumps,
>>> 	    show_total_period;
>>> } annotate_browser__opts = {
>>>          .use_offset      = true,
>>>          .jump_arrows     = true,
>>> };
>>>
>>> But if we use new config arrays that have all default config key-value pairs,
>>> we could initialize default config values with them.
>>>
>>> If we do, we can manage default perf config values at one spot (like util/config.c)
>>> and It can be easy and simple to modify default config values or add new configs.
>>>
>>> For example,
>>> If we use new default config arrays and there isn't user config value for 'annoate.use_offset'
>>> default value for it will be set as annotate_config_items[CONFIG_ANNOATE_USE_OFFSET].value
>>> instead of actual boolean type value 'true'.
>>>
>>> IMHO, I think it would needed to use new default config arrays
>>> to manage default perf config values more effectively.
>>> And this pathset contains patchs for only 'colors' and 'annoate' section
>>> because waiting for other opinions.
>>>
>>> If you review this patchset, I'd appreciate it :-)
>>>
>>> Thanks,
>>> Taeung
>>>
>>> v5:
>>> - rebased on current acme/perf/core
>>>
>>> v4:
>>> - rename 'fb_ground' to 'fore_back_colors' (Namhyung)
>>> - add struct default_config_section
>>> - split first patch[PATCH 1/7] as two
>>> - remove perf_default_config_init() at perf.c
>>> - rebased on current acme/perf/core
>>>
>>> v3:
>>> - remove default config arrays for the rest sections except 'colors' and 'annotate'
>>> - use combined {fore, back}ground colors instead of each two color
>>> - introduce perf_default_config_init() that call all default_*_config_init()
>>>     for each config section
>>>
>>> v2:
>>> - rename 'ui_browser__config_gcolors' to 'ui_browser__config_colors' (Arnaldo)
>>> - change 'ground colors' to '{back, fore}ground colors' (Arnaldo)
>>> - use strtok + ltrim instead of strchr and while (isspace(*++bg)); (Arnaldo)
>>>
>>> Taeung Song (7):
>>>     perf config: Introduce default_config_section and default_config_item
>>>       for default config key-value pairs
>>>     perf config: Add macros assigning key-value pairs to
>>>       default_config_item
>>>     perf config: Add 'colors' section default configs arrrays
>>>     perf config: Use combined {fore,back}ground colors value instead of
>>>       each two color
>>>     perf config: Initialize ui_browser__colorsets with default config
>>>       items
>>>     perf config: Add 'annotate' section default configs arrrays
>>>     perf config: Initialize annotate_browser__opts with default config
>>>       items
>>>
>>>    tools/perf/ui/browser.c           | 64 +++++++++++++++++-------------
>>>    tools/perf/ui/browsers/annotate.c | 16 ++++++--
>>>    tools/perf/util/config.c          | 26 +++++++++++++
>>>    tools/perf/util/config.h          | 82 +++++++++++++++++++++++++++++++++++++++
>>>    4 files changed, 156 insertions(+), 32 deletions(-)
>>>