Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo,

On Thu, Jan 21, 2016 at 11:02:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> > On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > 
> > SNIP
> > 
> > > > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > > > +		if (new == NULL)
> > > > > +			break;
> 
> Also, can we rename 'new' to new_he? In the past I used 'self' and
> Thomas rightly told me that 'self' didn't convey any info, likewise for
> 'new' (that is even a keyword in C++ and may confuse some syntax
> highligting, etc).

OK


> 
> > > > so hierarchy_insert_entry can fail because of memory allocation
> > > > but the resort path does not cover any error path because it only
> > > > shuffles entries from in-tree into sorted tree
> 
> > > Yes, memory allocation can fail anywhere.  If it happens, there's not
> > > much thing we can do IMHO - just print warning and bail out.
> > > Currently it silently ignores the allocation error and try to proceed.
> > > But I guess it'll fail soon at other place anyway.
> > 
> > I thought the 'policy' is to handle all allocation failures
> 
> yup, if some place doesn't, we need to fix it, silently trowing away
> stuff is not good. At the very least count the number of failures and
> inform the user somewhere on the screen.

OK, will change.


>  
> > > AFAICS current code also can fail in callchain_merge()..
> 
> That needs to be fixed too, then.

OK

Thanks,
Namhyung