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

[Arnaldo Carvalho de Melo]

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).

> > > 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.
 
> > AFAICS current code also can fail in callchain_merge()..

That needs to be fixed too, then.

- Arnaldo