Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1618834 > unrolled thread

[PATCH 0/5] Refactoring with ltrim() and rtrim()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] Refactoring with ltrim() and rtrim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-07 16:30 +0200
    [PATCH 2/5] perf stat: Refactor the code to strip csv output with ltrim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-07 16:30 +0200
      Re: [PATCH 2/5] perf stat: Refactor the code to strip csv output  with ltrim() Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-04-07 17:10 +0200
        Re: [PATCH 2/5] perf stat: Refactor the code to strip csv output with  ltrim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-08 01:50 +0200
    [PATCH 3/5] perf ui browser: Refactor the code to parse color configs with ltrim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-07 16:30 +0200
    [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-07 16:30 +0200
      Re: [PATCH 1/5] perf annotate: Refactor the code to parse  disassemble lines with {l,r}trim() Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-04-07 17:10 +0200
        Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble  lines with {l,r}trim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-07 20:10 +0200
      Re: [PATCH 1/5] perf annotate: Refactor the code to parse  disassemble lines with {l,r}trim() Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-04-07 17:10 +0200
        Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble  lines with {l,r}trim() Taeung Song <treeze.taeung@gmail.com> - 2017-04-08 02:20 +0200
      Re: [PATCH 1/5] perf annotate: Refactor the code to parse  disassemble lines with {l,r}trim() Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-04-07 17:10 +0200

#1618834 — [PATCH 0/5] Refactoring with ltrim() and rtrim()

Hi, :)

It is to simply refactor the code about stip strings
with ltrim() and rtrim().

I'd appreciate some feedback on this PATCHset.

Thanks,
Taeung

Taeung Song (5):
  perf annotate: Refactor the code to parse disassemble lines with
    {l,r}trim()
  perf stat: Refactor the code to strip csv output with ltrim()
  perf ui browser: Refactor the code to parse color configs with ltrim()
  perf pmu: Refactor wordwrap() with ltrim()
  perf tools: Refactor the code to strip command name with {l,r}trim()

 tools/perf/builtin-stat.c  | 10 ++--------
 tools/perf/ui/browser.c    |  2 +-
 tools/perf/util/annotate.c | 49 ++++++++++------------------------------------
 tools/perf/util/event.c    | 11 ++---------
 tools/perf/util/pmu.c      |  3 +--
 5 files changed, 16 insertions(+), 59 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1618835 — [PATCH 2/5] perf stat: Refactor the code to strip csv output with ltrim()

To strip csv output, use ltrim() instead of
just while loop and isspace() at print_metric_{only}_csv().

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-stat.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2158ea1..868e086a 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -875,10 +875,7 @@ static void print_metric_csv(void *ctx,
 		return;
 	}
 	snprintf(buf, sizeof(buf), fmt, val);
-	vals = buf;
-	while (isspace(*vals))
-		vals++;
-	ends = vals;
+	ends = vals = ltrim(buf);
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;
@@ -950,10 +947,7 @@ static void print_metric_only_csv(void *ctx, const char *color __maybe_unused,
 		return;
 	unit = fixunit(tbuf, os->evsel, unit);
 	snprintf(buf, sizeof buf, fmt, val);
-	vals = buf;
-	while (isspace(*vals))
-		vals++;
-	ends = vals;
+	ends = vals = ltrim(buf);
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1618886 — Re: [PATCH 2/5] perf stat: Refactor the code to strip csv output with ltrim()

Em Fri, Apr 07, 2017 at 11:24:18PM +0900, Taeung Song escreveu:
> To strip csv output, use ltrim() instead of
> just while loop and isspace() at print_metric_{only}_csv().

Applied.
 
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-stat.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 2158ea1..868e086a 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -875,10 +875,7 @@ static void print_metric_csv(void *ctx,
>  		return;
>  	}
>  	snprintf(buf, sizeof(buf), fmt, val);
> -	vals = buf;
> -	while (isspace(*vals))
> -		vals++;
> -	ends = vals;
> +	ends = vals = ltrim(buf);
>  	while (isdigit(*ends) || *ends == '.')
>  		ends++;
>  	*ends = 0;
> @@ -950,10 +947,7 @@ static void print_metric_only_csv(void *ctx, const char *color __maybe_unused,
>  		return;
>  	unit = fixunit(tbuf, os->evsel, unit);
>  	snprintf(buf, sizeof buf, fmt, val);
> -	vals = buf;
> -	while (isspace(*vals))
> -		vals++;
> -	ends = vals;
> +	ends = vals = ltrim(buf);
>  	while (isdigit(*ends) || *ends == '.')
>  		ends++;
>  	*ends = 0;
> -- 
> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1619162 — Re: [PATCH 2/5] perf stat: Refactor the code to strip csv output with ltrim()

On 04/08/2017 12:06 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 07, 2017 at 11:24:18PM +0900, Taeung Song escreveu:
>> To strip csv output, use ltrim() instead of
>> just while loop and isspace() at print_metric_{only}_csv().
>
> Applied.

Thank you!
  - Taeung

[toc] | [prev] | [next] | [standalone]

#1618839 — [PATCH 3/5] perf ui browser: Refactor the code to parse color configs with ltrim()

When parsing {fore, back} ground color configs,
use ltrim() instead of just while loop and isspace().

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/ui/browser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 3eb3edb..9e47ccb 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -579,7 +579,7 @@ static int ui_browser__color_config(const char *var, const char *value,
 			break;
 
 		*bg = '\0';
-		while (isspace(*++bg));
+		bg = ltrim(++bg);
 		ui_browser__colorsets[i].bg = bg;
 		ui_browser__colorsets[i].fg = fg;
 		return 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1618843 — [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim()

When parsing disassemble lines,
use ltrim() and rtrim() to strip them,
not using just while loop and isspace().

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/annotate.c | 49 ++++++++++------------------------------------
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index a37032b..1b4f17b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -379,9 +379,7 @@ static int mov__parse(struct arch *arch, struct ins_operands *ops, struct map *m
 	if (comment == NULL)
 		return 0;
 
-	while (comment[0] != '\0' && isspace(comment[0]))
-		++comment;
-
+	comment = ltrim(comment);
 	comment__symbol(ops->source.raw, comment, &ops->source.addr, &ops->source.name);
 	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
 
@@ -426,9 +424,7 @@ static int dec__parse(struct arch *arch __maybe_unused, struct ins_operands *ops
 	if (comment == NULL)
 		return 0;
 
-	while (comment[0] != '\0' && isspace(comment[0]))
-		++comment;
-
+	comment = ltrim(comment);
 	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
 
 	return 0;
@@ -777,10 +773,7 @@ static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, str
 
 static int disasm_line__parse(char *line, const char **namep, char **rawp)
 {
-	char *name = line, tmp;
-
-	while (isspace(name[0]))
-		++name;
+	char tmp, *name = ltrim(line);
 
 	if (name[0] == '\0')
 		return -1;
@@ -798,12 +791,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 		goto out_free_name;
 
 	(*rawp)[0] = tmp;
-
-	if ((*rawp)[0] != '\0') {
-		(*rawp)++;
-		while (isspace((*rawp)[0]))
-			++(*rawp);
-	}
+	*rawp = ltrim(*rawp);
 
 	return 0;
 
@@ -1148,9 +1136,9 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 {
 	struct annotation *notes = symbol__annotation(sym);
 	struct disasm_line *dl;
-	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
+	char *line = NULL, *parsed_line, *tmp, *tmp2;
 	size_t line_len;
-	s64 line_ip, offset = -1;
+	s64 line_ip = -1, offset = -1;
 	regmatch_t match[2];
 
 	if (getline(&line, &line_len, file) < 0)
@@ -1159,32 +1147,15 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	if (!line)
 		return -1;
 
-	while (line_len != 0 && isspace(line[line_len - 1]))
-		line[--line_len] = '\0';
-
-	c = strchr(line, '\n');
-	if (c)
-		*c = 0;
-
-	line_ip = -1;
-	parsed_line = line;
+	parsed_line = rtrim(line);
 
 	/* /filename:linenr ? Save line number and ignore. */
-	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
-		*line_nr = atoi(line + match[1].rm_so);
+	if (regexec(&file_lineno, parsed_line, 2, match, 0) == 0) {
+		*line_nr = atoi(parsed_line + match[1].rm_so);
 		return 0;
 	}
 
-	/*
-	 * Strip leading spaces:
-	 */
-	tmp = line;
-	while (*tmp) {
-		if (*tmp != ' ')
-			break;
-		tmp++;
-	}
-
+	tmp = ltrim(parsed_line);
 	if (*tmp) {
 		/*
 		 * Parse hexa addresses followed by ':'
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1618881 — Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim()

Em Fri, Apr 07, 2017 at 12:01:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Apr 07, 2017 at 11:24:17PM +0900, Taeung Song escreveu:
> > @@ -1148,9 +1136,9 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
> >  {
> >  	struct annotation *notes = symbol__annotation(sym);
> >  	struct disasm_line *dl;
> > -	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
> > +	char *line = NULL, *parsed_line, *tmp, *tmp2;
> >  	size_t line_len;
> > -	s64 line_ip, offset = -1;
> > +	s64 line_ip = -1, offset = -1;
> 
> Try to avoid doing these unrelated changes, i.e. moving the setting of
> line_ip to -1 from down below to here.
> 
> It is unrelated to what you're doing here, i.e. using ltrim/rtrim, and
> requires looking at the code to see if this can be done, as I don't know
> if line_ip is set to something else in-between... I am removing this,
> leaving the patch just for rtrim/ltrim.

Ok, please do it yourself, after taking into account the other comment
on this patch, I'm looking at the other patches now.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1619017 — Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim()

On 04/08/2017 12:04 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 07, 2017 at 12:01:30PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Fri, Apr 07, 2017 at 11:24:17PM +0900, Taeung Song escreveu:
>>> @@ -1148,9 +1136,9 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>>>  {
>>>  	struct annotation *notes = symbol__annotation(sym);
>>>  	struct disasm_line *dl;
>>> -	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
>>> +	char *line = NULL, *parsed_line, *tmp, *tmp2;
>>>  	size_t line_len;
>>> -	s64 line_ip, offset = -1;
>>> +	s64 line_ip = -1, offset = -1;
>>
>> Try to avoid doing these unrelated changes, i.e. moving the setting of
>> line_ip to -1 from down below to here.
>>
>> It is unrelated to what you're doing here, i.e. using ltrim/rtrim, and
>> requires looking at the code to see if this can be done, as I don't know
>> if line_ip is set to something else in-between... I am removing this,
>> leaving the patch just for rtrim/ltrim.
>
> Ok, please do it yourself, after taking into account the other comment
> on this patch, I'm looking at the other patches now.
>
> - Arnaldo
>

Thank you for detailed feedback.
I understood. As you said, I'll remove unrelated things in this patch.
And I'll send v2 based on your comment.

Thanks,
Taeung

[toc] | [prev] | [next] | [standalone]

#1618882 — Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim()

Em Fri, Apr 07, 2017 at 11:24:17PM +0900, Taeung Song escreveu:
> When parsing disassemble lines,
> use ltrim() and rtrim() to strip them,
> not using just while loop and isspace().
> 
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/annotate.c | 49 ++++++++++------------------------------------
>  1 file changed, 10 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index a37032b..1b4f17b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -379,9 +379,7 @@ static int mov__parse(struct arch *arch, struct ins_operands *ops, struct map *m
>  	if (comment == NULL)
>  		return 0;
>  
> -	while (comment[0] != '\0' && isspace(comment[0]))
> -		++comment;
> -
> +	comment = ltrim(comment);
>  	comment__symbol(ops->source.raw, comment, &ops->source.addr, &ops->source.name);
>  	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
>  
> @@ -426,9 +424,7 @@ static int dec__parse(struct arch *arch __maybe_unused, struct ins_operands *ops
>  	if (comment == NULL)
>  		return 0;
>  
> -	while (comment[0] != '\0' && isspace(comment[0]))
> -		++comment;
> -
> +	comment = ltrim(comment);
>  	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
>  
>  	return 0;
> @@ -777,10 +773,7 @@ static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, str
>  
>  static int disasm_line__parse(char *line, const char **namep, char **rawp)
>  {
> -	char *name = line, tmp;
> -
> -	while (isspace(name[0]))
> -		++name;
> +	char tmp, *name = ltrim(line);
>  
>  	if (name[0] == '\0')
>  		return -1;
> @@ -798,12 +791,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
>  		goto out_free_name;
>  
>  	(*rawp)[0] = tmp;
> -
> -	if ((*rawp)[0] != '\0') {
> -		(*rawp)++;
> -		while (isspace((*rawp)[0]))
> -			++(*rawp);
> -	}
> +	*rawp = ltrim(*rawp);
>  
>  	return 0;
>  
> @@ -1148,9 +1136,9 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  {
>  	struct annotation *notes = symbol__annotation(sym);
>  	struct disasm_line *dl;
> -	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
> +	char *line = NULL, *parsed_line, *tmp, *tmp2;
>  	size_t line_len;
> -	s64 line_ip, offset = -1;
> +	s64 line_ip = -1, offset = -1;
>  	regmatch_t match[2];
>  
>  	if (getline(&line, &line_len, file) < 0)
> @@ -1159,32 +1147,15 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  	if (!line)
>  		return -1;
>  
> -	while (line_len != 0 && isspace(line[line_len - 1]))
> -		line[--line_len] = '\0';
> -
> -	c = strchr(line, '\n');
> -	if (c)
> -		*c = 0;
> -
> -	line_ip = -1;
> -	parsed_line = line;
> +	parsed_line = rtrim(line);
>  
>  	/* /filename:linenr ? Save line number and ignore. */
> -	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
> -		*line_nr = atoi(line + match[1].rm_so);
> +	if (regexec(&file_lineno, parsed_line, 2, match, 0) == 0) {
> +		*line_nr = atoi(parsed_line + match[1].rm_so);

Both should be the same, so this is further noise in the patch, please
refrain from doing extra things in a patch. If you feel this is really
needed, do it in _another_ patch, with proper explanation.

This is a trivial case, but you need to be consistent, avoiding to lump
together unrelated stuff in the same patch.

>  		return 0;
>  	}
>  
> -	/*
> -	 * Strip leading spaces:
> -	 */
> -	tmp = line;
> -	while (*tmp) {
> -		if (*tmp != ' ')
> -			break;
> -		tmp++;
> -	}
> -
> +	tmp = ltrim(parsed_line);
>  	if (*tmp) {
>  		/*
>  		 * Parse hexa addresses followed by ':'
> -- 
> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1619167 — Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim()

On 04/08/2017 12:04 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 07, 2017 at 11:24:17PM +0900, Taeung Song escreveu:
>> When parsing disassemble lines,
>> use ltrim() and rtrim() to strip them,
>> not using just while loop and isspace().
>>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/util/annotate.c | 49 ++++++++++------------------------------------
>>  1 file changed, 10 insertions(+), 39 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index a37032b..1b4f17b 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -379,9 +379,7 @@ static int mov__parse(struct arch *arch, struct ins_operands *ops, struct map *m
>>  	if (comment == NULL)
>>  		return 0;
>>
>> -	while (comment[0] != '\0' && isspace(comment[0]))
>> -		++comment;
>> -
>> +	comment = ltrim(comment);
>>  	comment__symbol(ops->source.raw, comment, &ops->source.addr, &ops->source.name);
>>  	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
>>
>> @@ -426,9 +424,7 @@ static int dec__parse(struct arch *arch __maybe_unused, struct ins_operands *ops
>>  	if (comment == NULL)
>>  		return 0;
>>
>> -	while (comment[0] != '\0' && isspace(comment[0]))
>> -		++comment;
>> -
>> +	comment = ltrim(comment);
>>  	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
>>
>>  	return 0;
>> @@ -777,10 +773,7 @@ static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, str
>>
>>  static int disasm_line__parse(char *line, const char **namep, char **rawp)
>>  {
>> -	char *name = line, tmp;
>> -
>> -	while (isspace(name[0]))
>> -		++name;
>> +	char tmp, *name = ltrim(line);
>>
>>  	if (name[0] == '\0')
>>  		return -1;
>> @@ -798,12 +791,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
>>  		goto out_free_name;
>>
>>  	(*rawp)[0] = tmp;
>> -
>> -	if ((*rawp)[0] != '\0') {
>> -		(*rawp)++;
>> -		while (isspace((*rawp)[0]))
>> -			++(*rawp);
>> -	}
>> +	*rawp = ltrim(*rawp);
>>
>>  	return 0;
>>
>> @@ -1148,9 +1136,9 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>>  {
>>  	struct annotation *notes = symbol__annotation(sym);
>>  	struct disasm_line *dl;
>> -	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
>> +	char *line = NULL, *parsed_line, *tmp, *tmp2;
>>  	size_t line_len;
>> -	s64 line_ip, offset = -1;
>> +	s64 line_ip = -1, offset = -1;
>>  	regmatch_t match[2];
>>
>>  	if (getline(&line, &line_len, file) < 0)
>> @@ -1159,32 +1147,15 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>>  	if (!line)
>>  		return -1;
>>
>> -	while (line_len != 0 && isspace(line[line_len - 1]))
>> -		line[--line_len] = '\0';
>> -
>> -	c = strchr(line, '\n');
>> -	if (c)
>> -		*c = 0;
>> -
>> -	line_ip = -1;
>> -	parsed_line = line;
>> +	parsed_line = rtrim(line);
>>
>>  	/* /filename:linenr ? Save line number and ignore. */
>> -	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
>> -		*line_nr = atoi(line + match[1].rm_so);
>> +	if (regexec(&file_lineno, parsed_line, 2, match, 0) == 0) {
>> +		*line_nr = atoi(parsed_line + match[1].rm_so);
>
> Both should be the same, so this is further noise in the patch, please
> refrain from doing extra things in a patch. If you feel this is really
> needed, do it in _another_ patch, with proper explanation.
>
> This is a trivial case, but you need to be consistent, avoiding to lump
> together unrelated stuff in the same patch.

Thank you for detailed advice!
will do as you comment.

  - Taeung

>
>>  		return 0;
>>  	}
>>
>> -	/*
>> -	 * Strip leading spaces:
>> -	 */
>> -	tmp = line;
>> -	while (*tmp) {
>> -		if (*tmp != ' ')
>> -			break;
>> -		tmp++;
>> -	}
>> -
>> +	tmp = ltrim(parsed_line);
>>  	if (*tmp) {
>>  		/*
>>  		 * Parse hexa addresses followed by ':'
>> --
>> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1618887 — Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim()

Em Fri, Apr 07, 2017 at 11:24:17PM +0900, Taeung Song escreveu:
> When parsing disassemble lines,
> use ltrim() and rtrim() to strip them,
> not using just while loop and isspace().
> 
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/annotate.c | 49 ++++++++++------------------------------------
>  1 file changed, 10 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index a37032b..1b4f17b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -379,9 +379,7 @@ static int mov__parse(struct arch *arch, struct ins_operands *ops, struct map *m
>  	if (comment == NULL)
>  		return 0;
>  
> -	while (comment[0] != '\0' && isspace(comment[0]))
> -		++comment;
> -
> +	comment = ltrim(comment);
>  	comment__symbol(ops->source.raw, comment, &ops->source.addr, &ops->source.name);
>  	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
>  
> @@ -426,9 +424,7 @@ static int dec__parse(struct arch *arch __maybe_unused, struct ins_operands *ops
>  	if (comment == NULL)
>  		return 0;
>  
> -	while (comment[0] != '\0' && isspace(comment[0]))
> -		++comment;
> -
> +	comment = ltrim(comment);
>  	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
>  
>  	return 0;
> @@ -777,10 +773,7 @@ static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, str
>  
>  static int disasm_line__parse(char *line, const char **namep, char **rawp)
>  {
> -	char *name = line, tmp;
> -
> -	while (isspace(name[0]))
> -		++name;
> +	char tmp, *name = ltrim(line);
>  
>  	if (name[0] == '\0')
>  		return -1;
> @@ -798,12 +791,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
>  		goto out_free_name;
>  
>  	(*rawp)[0] = tmp;
> -
> -	if ((*rawp)[0] != '\0') {
> -		(*rawp)++;
> -		while (isspace((*rawp)[0]))
> -			++(*rawp);
> -	}
> +	*rawp = ltrim(*rawp);
>  
>  	return 0;
>  
> @@ -1148,9 +1136,9 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  {
>  	struct annotation *notes = symbol__annotation(sym);
>  	struct disasm_line *dl;
> -	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
> +	char *line = NULL, *parsed_line, *tmp, *tmp2;
>  	size_t line_len;
> -	s64 line_ip, offset = -1;
> +	s64 line_ip = -1, offset = -1;

Try to avoid doing these unrelated changes, i.e. moving the setting of
line_ip to -1 from down below to here.

It is unrelated to what you're doing here, i.e. using ltrim/rtrim, and
requires looking at the code to see if this can be done, as I don't know
if line_ip is set to something else in-between... I am removing this,
leaving the patch just for rtrim/ltrim.

>  	regmatch_t match[2];
>  
>  	if (getline(&line, &line_len, file) < 0)
> @@ -1159,32 +1147,15 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  	if (!line)
>  		return -1;
>  
> -	while (line_len != 0 && isspace(line[line_len - 1]))
> -		line[--line_len] = '\0';
> -
> -	c = strchr(line, '\n');
> -	if (c)
> -		*c = 0;
> -
> -	line_ip = -1;
> -	parsed_line = line;
> +	parsed_line = rtrim(line);
>  
>  	/* /filename:linenr ? Save line number and ignore. */
> -	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
> -		*line_nr = atoi(line + match[1].rm_so);
> +	if (regexec(&file_lineno, parsed_line, 2, match, 0) == 0) {
> +		*line_nr = atoi(parsed_line + match[1].rm_so);
>  		return 0;
>  	}
>  
> -	/*
> -	 * Strip leading spaces:
> -	 */
> -	tmp = line;
> -	while (*tmp) {
> -		if (*tmp != ' ')
> -			break;
> -		tmp++;
> -	}
> -
> +	tmp = ltrim(parsed_line);
>  	if (*tmp) {
>  		/*
>  		 * Parse hexa addresses followed by ':'
> -- 
> 2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web