Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755729AbdDHAMk (ORCPT ); Fri, 7 Apr 2017 20:12:40 -0400 Received: from mail-pg0-f66.google.com ([74.125.83.66]:35152 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753962AbdDHAMd (ORCPT ); Fri, 7 Apr 2017 20:12:33 -0400 Subject: Re: [PATCH 1/5] perf annotate: Refactor the code to parse disassemble lines with {l,r}trim() To: Arnaldo Carvalho de Melo References: <1491575061-704-1-git-send-email-treeze.taeung@gmail.com> <1491575061-704-2-git-send-email-treeze.taeung@gmail.com> <20170407150404.GD2966@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Wang Nan , Masami Hiramatsu From: Taeung Song Message-ID: <755157b3-740d-e4b9-e3a4-93f8741dad2b@gmail.com> Date: Sat, 8 Apr 2017 09:12:28 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170407150404.GD2966@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3969 Lines: 132 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 >> Cc: Namhyung Kim >> Signed-off-by: Taeung Song >> --- >> 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