Re: [PATCH v2 06/11] perf expr: parse numbers as doubles

[Ian Rogers]

On Mon, Apr 27, 2020 at 4:12 AM kajoljain <kjain@xxxxxxxxxxxxx> wrote:
>
> On 4/23/20 3:34 AM, Ian Rogers wrote:
> > This is expected in expr.y and metrics use floating point values such as
> > x86 broadwell IFetch_Line_Utilization.
> >
> > Fixes: 26226a97724d (perf expr: Move expr lexer to flex)
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/util/expr.l | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> > index 95bcf3629edf..0efda2ce2766 100644
> > --- a/tools/perf/util/expr.l
> > +++ b/tools/perf/util/expr.l
> > @@ -10,12 +10,12 @@
> >  char *expr_get_text(yyscan_t yyscanner);
> >  YYSTYPE *expr_get_lval(yyscan_t yyscanner);
> >
> > -static int __value(YYSTYPE *yylval, char *str, int base, int token)
> > +static double __value(YYSTYPE *yylval, char *str, int token)
> >  {
> > -     u64 num;
> > +     double num;
> >
> >       errno = 0;
> > -     num = strtoull(str, NULL, base);
> > +     num = strtod(str, NULL);
> >       if (errno)
> >               return EXPR_ERROR;
> >
> > @@ -23,12 +23,12 @@ static int __value(YYSTYPE *yylval, char *str, int base, int token)
> >       return token;
> >  }
> >
> > -static int value(yyscan_t scanner, int base)
> > +static int value(yyscan_t scanner)
> >  {
> >       YYSTYPE *yylval = expr_get_lval(scanner);
> >       char *text = expr_get_text(scanner);
> >
> > -     return __value(yylval, text, base, NUMBER);
> > +     return __value(yylval, text, NUMBER);
> >  }
> >
> >  /*
> > @@ -68,7 +68,7 @@ static int str(yyscan_t scanner, int token)
> >  }
> >  %}
> >
> > -number               [0-9]+
> > +number               [0-9]*\.?[0-9]+
> >
>
> Hi Ian,
>     In this patch I saw the parsing of expression with '+number         [0-9]*\.?[0-9]+'
> Could you please explain why '?' is introduced here, so that I can be sure that this is
> not conflicting with my change to add '?'

Hi Kajol,
the '?' here is part of the regular expression. Basically it is saying
that a number is a possible set of integers possibly followed by a '.'
and then 1 or more integers. The expression comes from having seen
Intel's metrics are of the form '.1234' in some of their topdown
spreadsheets, so we need to be able to handle cases like '1.2', '123'
and '.1234'. Having looked at your patch I don't believe it
interferes.

> In this patch : https://lkml.org/lkml/2020/4/1/1427
> I have used '?' symbol as part of metric expression in order to replace '?' with runtime
> parameter.

Interesting, I'll follow up with comments on that e-mail.

Thanks!
Ian

> Thanks,
> Kajol Jain
>
>
> >  sch          [-,=]
> >  spec         \\{sch}
> > @@ -92,7 +92,7 @@ min         { return MIN; }
> >  if           { return IF; }
> >  else         { return ELSE; }
> >  #smt_on              { return SMT_ON; }
> > -{number}     { return value(yyscanner, 10); }
> > +{number}     { return value(yyscanner); }
> >  {symbol}     { return str(yyscanner, ID); }
> >  "|"          { return '|'; }
> >  "^"          { return '^'; }
> >