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
|