Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc.

["Satyam Sharma" <satyam.sharma@gmail.com>]

On 5/9/07, Pallipadi, Venkatesh <venkatesh.pallipadi@intel.com> wrote:
>
> >-----Original Message-----
> >From: Jarek Poplawski [mailto:jarkao2@o2.pl]
> >Sent: Tuesday, May 08, 2007 10:32 PM
> >To: Andrew Morton
> >Cc: Pallipadi, Venkatesh; linux-kernel@vger.kernel.org; Oleg Nesterov
> >Subject: Re: [PATCH -mm] timer: parenthesis fix in
> >tbase_get_deferrable() etc.
> >
> >On Tue, May 08, 2007 at 04:33:58PM -0700, Andrew Morton wrote:
> >> On Tue, 8 May 2007 12:33:48 +0200
> >> Jarek Poplawski <jarkao2@o2.pl> wrote:
> >>
> >> >
> >> > Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
> >> >
> >> > ---
> >> >
> >> > diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
> >> > --- 2.6.21-mm1-/kernel/timer.c     2007-05-08
> >11:54:48.000000000 +0200
> >> > +++ 2.6.21-mm1/kernel/timer.c      2007-05-08
> >12:05:11.000000000 +0200
> >> > @@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
> >> >  /* Functions below help us manage 'deferrable' flag */
> >> >  static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
> >> >  {
> >> > -  return ((unsigned int)(unsigned long)base &
> >TBASE_DEFERRABLE_FLAG);
> >> > +  return (unsigned int)((unsigned long)base &
> >TBASE_DEFERRABLE_FLAG);
> >> >  }
> >...
> >> The change makes sense, but does it actually "fix" anything?
> >>
> >
> >Yes - this first place fixes logical error, so it's a sin
> >- even if not punishable in practice. (It's also unnecessary
> >test for long to int conversion.)
> >
>
> I am sorry, I don't understand. What is the logical error in the first
> one?
>
> Actually, your change makes it different from what was originally
> indended.
> Original intention was to type convert base to a 32 bit value and
> bitwise& with FLAG.

But that is not what the original code is doing. If you wanted to
typecast "base" to "a 32 bit value" then you should've used u32
instead.

Anyway, if you originally intended to actually typecast "base" to
unsigned int, then you could do that directly without typecasting it
first to unsigned long (unnecessarily) and then to unsigned int. Of
course, if your system implements a pointer as something bigger than
unsigned int (which is what you eventually convert "base" to), then
you're screwed anyway and the intermediate typecast to unsigned long
doesn't buy you anything at all.

The other 3 changes in this patch were clearly meaningless, though.