Re: [PATCH 2/4] perf tools: Separate out code setting ground colors from ui_browser__color_config

[Taeung Song]

Hi, Arnaldo :)

On 05/10/2016 02:17 AM, Arnaldo Carvalho de Melo wrote:
Em Mon, May 09, 2016 at 08:41:47PM +0900, Taeung Song escreveu:
ui_browser__color_config() set foreground and background
colors values in ui_browser__colorsets.

"ground colors" sounds strange, I guess referreing to them as "*colors"
or "{back,fore}ground colors" is more appropriate, replace "gcolors"
with "colors" too, please.

I got it.

But it can be reused by other functions so make ui_browser__config_gcolors()
bringing it from ui_browser__color_config().

Cc: Namhyung Kim <namhy...@kernel.org>
Cc: Jiri Olsa <jo...@kernel.org>
Signed-off-by: Taeung Song <treeze.tae...@gmail.com>
---
  tools/perf/ui/browser.c | 43 ++++++++++++++++++++++++++-----------------
  1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index af68a9d..a477867 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -553,12 +553,33 @@ static struct ui_browser_colorset {
        }
  };

+static int ui_browser__config_gcolors(struct ui_browser_colorset 
*ui_browser_color,
+                                     const char *value)
+{
+       char *fg = NULL, *bg;
+
+       fg = strdup(value);
+       if (fg == NULL)
+               return -1;
+
+       bg = strchr(fg, ',');
+       if (bg == NULL) {
+               free(fg);
+               return -1;
+       }
+
+       *bg = '\0';

Isn't the above strtok()?

+       while (isspace(*++bg));

Isn't this ltrim()?

I modified it like below and retested it !


diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index a477867..c905445 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -553,8 +553,8 @@ static struct ui_browser_colorset {
         }
 };

-static int ui_browser__config_gcolors(struct ui_browser_colorset 
*ui_browser_color,
-                                      const char *value)
+static int ui_browser__config_colors(struct ui_browser_colorset 
*ui_browser_color,
+                                     const char *value)
 {
         char *fg = NULL, *bg;

@@ -562,14 +562,12 @@ static int ui_browser__config_gcolors(struct 
ui_browser_colorset *ui_browser_col
         if (fg == NULL)
                 return -1;

-        bg = strchr(fg, ',');
+        bg = strtok(fg, ",");
         if (bg == NULL) {
                 free(fg);
                 return -1;
         }
-
-        *bg = '\0';
-        while (isspace(*++bg));
+        ltrim(bg);

         ui_browser_color->fg = fg;
         ui_browser_color->bg = bg;
@@ -591,7 +589,7 @@ static int ui_browser__color_config(const char *var, 
const char *value,
                 if (strcmp(ui_browser__colorsets[i].name, name) != 0)
                         continue;

-                ret = 
ui_browser__config_gcolors(&ui_browser__colorsets[i], value);
+                ret = 
ui_browser__config_colors(&ui_browser__colorsets[i], value);
                 break;
         }


I'll send this modified patch with v2


Thanks,
Taeung