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

From: Namhyung Kim
Date: Tue Jul 12 2016 - 01:38:51 EST


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(-)
> >