Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer

[Steven Rostedt <srostedt@redhat.com>]

On Thu, 2011-01-20 at 14:03 +0300, Sergei Shtylyov wrote:

> > diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> > index bc91e4a..62775d7 100644
> > --- a/arch/mips/kernel/ftrace.c
> > +++ b/arch/mips/kernel/ftrace.c
> [...]
> > @@ -304,7 +304,14 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
> >   		return;
> >   	}
> >
> > -	trace.func = self_ra;
> > +	/*
> > +	 * Get the recorded ip of the current mcount calling site in the
> > +	 * __mcount_loc section, which will be used to filter the function
> > +	 * entries configured through the tracing/set_graph_function interface.
> > +	 */
> > +
> > +	insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
> 
>     Unneeded parens.

agreed

> 
> > +	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
> 
>     Here too.

I like the parens here. Yes, it is basic math precedence, but it stands
out to a reviewer who has their brain more focused on correct code than
thinking about which op has precedence.

Reviewing code that has:

	trace.func = self_ra - MCOUNT_INSN_SIZE * insns;

And as I my mother language reads left to right, my thought process
goes: subtract MCOUNT_INSN_SIZE from self_ra and then times insns... oh
wait, times goes first; stop reset, restart... subtract MCOUNT_INSN_SIZE
times insns from self_ra.  That stop reset and restart of the thought
process breaks the train of thought and could waste a lot more time than
just the moment it happened. All this is avoided by the parenthesis that
automatically trigger the brain to think those go first.

I say, leave these in.

-- Steve