Re: [PATCH v8 3/4] perf config: Prepare all default configs

From: Taeung Song
Date: Wed Apr 20 2016 - 11:29:24 EST


Hi, Arnaldo :-)

On 04/20/2016 10:22 PM, Arnaldo Carvalho de Melo wrote:
Em Wed, Apr 20, 2016 at 09:44:38PM +0900, Namhyung Kim escreveu:
On Mon, Apr 18, 2016 at 11:55:18PM +0900, Taeung Song wrote:
On 04/15/2016 01:42 AM, Taeung Song wrote:
On 04/14/2016 09:19 PM, Arnaldo Carvalho de Melo wrote:
Em Thu, Apr 14, 2016 at 04:53:20PM +0900, Taeung Song escreveu:
+++ b/tools/perf/util/config.c
@@ -15,6 +15,7 @@
+#define MAX_CONFIGS 64

Do we have to add another arbitrary maximum? Where is it used?

IMHO, it is my idea. If you want to avoid using this arbitrary maxinum,
I'd modify the code.

MAX_CONFIGS is used in order to declare two-dimensional arrays
'default_config_items'

As above, I used MAX_CONFIGS because of two-dimensinal arrays
'default_config_items'.

What do you think about it ?

I also agree that we'd better to avoid the arbitrary maximum.

We don't need to add this arbitrary maximum ?
or would you mind, if I look for other way about
'default_config_item' ?

What about this?

Yeah, I guess this should work, no? At least this is how it is done
elsewhere, see:

tools/perf/builtin-bench.c, 'struct collection' has a benchmarks array,
that in turn is organized as Namhyung suggests.

I got it!

Then you either use ARRAY_SIZE() somewhere to get the number of entries
or use a sentinel, i.e. use NULL for the last entry.


I think that it would better to use ARRAY_SIZE() than to use NULL.
It makes no odds but it seem to be clean, IMHO. :)

After changing this patchset, I'll send v9 !

Thanks,
Taeung


struct perf_config_item color_config_items[] = {
CONF_STR_VAR("top", "red, default"),
CONF_STR_VAR("medium", "green, default"),
...
};

struct perf_config_item tui_config_items[] = {
CONF_BOOL_VAR("report", true),
CONF_BOOL_VAR("annotate", true),
...
};

struct perf_config_item *default_config_items[] = {
&color_config_items,
&tui_config_items,
...
};

This way we can access the config array by using constant index
without the hard-coded maximum size IMHO.

Thanks,
Namhyung