On Wed, Feb 27, 2019 at 09:04:21PM +0800, Jin, Yao wrote:
>
>
> On 2/27/2019 5:27 PM, Jiri Olsa wrote:
> > On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:
> >
> > SNIP
> >
> > > + abstime_tmp = abstime_ostr;
> > > data__for_each_file(i, d) {
> > > - d->session = perf_session__new(&d->data, false, &tool);
> > > + d->session = perf_session__new(&d->data, false, &pdiff.tool);
> > > if (!d->session) {
> > > pr_err("Failed to open %s\n", d->data.path);
> > > ret = -1;
> > > goto out_delete;
> > > }
> > > + if (abstime_ostr) {
> > > + ret = parse_absolute_time(d, &abstime_tmp);
> > > + if (ret < 0)
> > > + goto out_delete;
> > > + } else if (pdiff.time_str) {
> > > + ret = parse_percent_time(d);
> > > + if (ret < 0)
> > > + goto out_delete;
> > > + } else {
> > > + pdiff.range_num = 1;
> >
> > hum, why are we setting range_num to 1 again?
>
> Yes, that may be not necessary. I will remove this line and test again.
>
> >
> > it's really hard to parse this code, maybe
> > it'd be better in separate loop/function
> > that would setup just timestamps..
> >
>
> Do you mean the above parsing code should be put in a separate function
> (e.g. parse_time_string in following example)?
>
> data__for_each_file(i, d) {
> ....
> d->session = perf_session__new(&d->data, false, &pdiff.tool);
> ....
> parse_time_string(...);
> ret = perf_session__process_events(d->session);
> ....
> }
anything that would make this more clear/readable ;-)
thanks,
jirka
