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

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

[Jarek Poplawski]

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);
 }
 
 static inline tvec_base_t *tbase_get_base(tvec_base_t *base)
 {
-	return ((tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG));
+	return (tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG);
 }
 
 static inline void timer_set_deferrable(struct timer_list *timer)
 {
-	timer->base = ((tvec_base_t *)((unsigned long)(timer->base) |
-	                               TBASE_DEFERRABLE_FLAG));
+	timer->base = (tvec_base_t *)((unsigned long)timer->base |
+	                               TBASE_DEFERRABLE_FLAG);
 }
 
 static inline void
 timer_set_base(struct timer_list *timer, tvec_base_t *new_base)
 {
-	timer->base = (tvec_base_t *)((unsigned long)(new_base) |
+	timer->base = (tvec_base_t *)((unsigned long)new_base |
 	                              tbase_get_deferrable(timer->base));
 }
 

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

[Andrew Morton]

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);
>  }
>  
>  static inline tvec_base_t *tbase_get_base(tvec_base_t *base)
>  {
> -	return ((tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG));
> +	return (tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG);
>  }
>  
>  static inline void timer_set_deferrable(struct timer_list *timer)
>  {
> -	timer->base = ((tvec_base_t *)((unsigned long)(timer->base) |
> -	                               TBASE_DEFERRABLE_FLAG));
> +	timer->base = (tvec_base_t *)((unsigned long)timer->base |
> +	                               TBASE_DEFERRABLE_FLAG);
>  }
>  
>  static inline void
>  timer_set_base(struct timer_list *timer, tvec_base_t *new_base)
>  {
> -	timer->base = (tvec_base_t *)((unsigned long)(new_base) |
> +	timer->base = (tvec_base_t *)((unsigned long)new_base |
>  	                              tbase_get_deferrable(timer->base));
>  }
>  

The change makes sense, but does it actually "fix" anything?

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

[Jarek Poplawski]

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

Other changes are only BTW.

Regards,
Jarek P.

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

[Pallipadi, Venkatesh]

>-----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. Even though compiler may optimize both the above to
same code, I don't see what is the error.

Thanks,
Venki 

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

[Satyam Sharma]

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.

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

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Satyam Sharma [mailto:satyam.sharma@gmail.com] 
>Sent: Wednesday, May 09, 2007 11:30 AM
>To: Pallipadi, Venkatesh
>Cc: Jarek Poplawski; Andrew Morton; 
>linux-kernel@vger.kernel.org; Oleg Nesterov
>Subject: Re: [PATCH -mm] timer: parenthesis fix in 
>tbase_get_deferrable() etc.
>
>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.

On a 64 bit system, converting pointer to int causes unnecessary
compiler
warning, and intermediate long conversion was to avoid that. I will have
to rephrase my comment to remove 32 bit value and use int, as that is
what
the function returns.

Thanks,
Venki

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

[Jarek Poplawski]

On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
...
> On a 64 bit system, converting pointer to int causes unnecessary
> compiler
> warning, and intermediate long conversion was to avoid that. I will have
> to rephrase my comment to remove 32 bit value and use int, as that is
> what
> the function returns.

Sorry!!! So, it's perfectly right and logical.

It's a pity, you couldn't NACK this patch a little sooner.
I hope there is no problem to remove this patch now. 

It seems this type of conversion isn't popular enough, yet.

Thanks for explanation & regards,
Jarek P.

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

[Andrew Morton]

On Thu, 10 May 2007 09:39:04 +0200 Jarek Poplawski <jarkao2@o2.pl> wrote:

> On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> ...
> > On a 64 bit system, converting pointer to int causes unnecessary
> > compiler
> > warning, and intermediate long conversion was to avoid that. I will have
> > to rephrase my comment to remove 32 bit value and use int, as that is
> > what
> > the function returns.
> 
> Sorry!!! So, it's perfectly right and logical.
> 
> It's a pity, you couldn't NACK this patch a little sooner.
> I hope there is no problem to remove this patch now. 

Please send a new patch.  That way we get a nice explanation for the
reversion, and we have well-oiled processes for applying patches, but
crappy processes for reverting them.

[PATCH] timer: revert parenthesis fix in tbase_get_deferrable() etc.

[Jarek Poplawski]

On Thu, May 10, 2007 at 12:57:38AM -0700, Andrew Morton wrote:
> On Thu, 10 May 2007 09:39:04 +0200 Jarek Poplawski <jarkao2@o2.pl> wrote:
> 
> > On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> > ...
> > > On a 64 bit system, converting pointer to int causes unnecessary
> > > compiler
> > > warning, and intermediate long conversion was to avoid that. I will have
> > > to rephrase my comment to remove 32 bit value and use int, as that is
> > > what
> > > the function returns.
> > 
> > Sorry!!! So, it's perfectly right and logical.
> > 
> > It's a pity, you couldn't NACK this patch a little sooner.
> > I hope there is no problem to remove this patch now. 
> 
> Please send a new patch.  That way we get a nice explanation for the
> reversion, and we have well-oiled processes for applying patches, but
> crappy processes for reverting them.
> 

--->

[PATCH] timer: revert parenthesis fix in tbase_get_deferrable() etc.

> > On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> > ...
> > > On a 64 bit system, converting pointer to int causes unnecessary
> > > compiler
> > > warning, and intermediate long conversion was to avoid that. I will have
> > > to rephrase my comment to remove 32 bit value and use int, as that is
> > > what
> > > the function returns.

So, this patch reverts all changes done by my previous patch.

I appologize for my wrong comment about "logical error" here.


Cc: "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>
Cc: Satyam Sharma <satyam.sharma@gmail.com>
Cc: Oleg Nesterov <oleg@tv-sign.ru>

Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>

---

diff -Nurp 2.6.21-git12-/kernel/timer.c 2.6.21-git12/kernel/timer.c
--- 2.6.21-git12-/kernel/timer.c	2007-05-10 10:55:34.000000000 +0200
+++ 2.6.21-git12/kernel/timer.c	2007-05-10 11:02:36.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);
 }
 
 static inline tvec_base_t *tbase_get_base(tvec_base_t *base)
 {
-	return (tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG);
+	return ((tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG));
 }
 
 static inline void timer_set_deferrable(struct timer_list *timer)
 {
-	timer->base = (tvec_base_t *)((unsigned long)timer->base |
-	                               TBASE_DEFERRABLE_FLAG);
+	timer->base = ((tvec_base_t *)((unsigned long)(timer->base) |
+	                               TBASE_DEFERRABLE_FLAG));
 }
 
 static inline void
 timer_set_base(struct timer_list *timer, tvec_base_t *new_base)
 {
-	timer->base = (tvec_base_t *)((unsigned long)new_base |
+	timer->base = (tvec_base_t *)((unsigned long)(new_base) |
 	                              tbase_get_deferrable(timer->base));
 }
 

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

[Jarek Poplawski]

On Thu, May 10, 2007 at 11:29:47AM +0200, Jarek Poplawski wrote:
...
> [PATCH] timer: revert parenthesis fix in tbase_get_deferrable() etc.
> 
> > > On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> > > ...
> > > > On a 64 bit system, converting pointer to int causes unnecessary
> > > > compiler
> > > > warning, and intermediate long conversion was to avoid that. I will have
> > > > to rephrase my comment to remove 32 bit value and use int, as that is
> > > > what
> > > > the function returns.
> 
> So, this patch reverts all changes done by my previous patch.
> 
> I appologize for my wrong comment about "logical error" here.

Should be:

I apologize for my wrong comment about "logical error" here.

Jarek P.

PS: Why I couldn't be sorry, as usual...

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

[Satyam Sharma]

> >> >> >  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.
>
> On a 64 bit system, converting pointer to int causes unnecessary
> compiler
> warning, and intermediate long conversion was to avoid that. I will have

Whoa! Hello, hold on, just wait a second there. Do you _really_ want
an unsigned int return out of tbase_get_deferrable() or will an
unsigned long do? If the rest of your code is fine with unsigned long,
then I'd suggest something like:

static inline unsigned long tbase_get_deferrable(tvec_base_t *base)
{
	return ((unsigned long)base & TBASE_DEFERRABLE_FLAG);
}

I don't really know your code (so I could be horribly incorrect here),
but personally I would prefer *heeding* to that warning than _hiding_
it -- it's not unnecessary, it's telling you that you're *losing* data
by converting a pointer (which is always unsigned long) to unsigned
int for 64-bit platforms where sizeof(void *) == sizeof(unsigned long)
== 8 bytes, but sizeof(unsigned int) == 4.

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

[Satyam Sharma]

On 5/10/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> > [...]
> > On a 64 bit system, converting pointer to int causes unnecessary
> > compiler
> > warning, and intermediate long conversion was to avoid that. I will have
>
> Whoa! Hello, hold on, just wait a second there. Do you _really_ want
> an unsigned int return out of tbase_get_deferrable() or will an
> unsigned long do? If the rest of your code is fine with unsigned long,
> then I'd suggest something like:
>
> static inline unsigned long tbase_get_deferrable(tvec_base_t *base)
> {
>         return ((unsigned long)base & TBASE_DEFERRABLE_FLAG);
> }
>
> I don't really know your code (so I could be horribly incorrect here),
> but personally I would prefer *heeding* to that warning than _hiding_
> it -- it's not unnecessary, it's telling you that you're *losing* data
> by converting a pointer (which is always unsigned long) to unsigned
> int for 64-bit platforms where sizeof(void *) == sizeof(unsigned long)
> == 8 bytes, but sizeof(unsigned int) == 4.

Oh well, pardon my obtuseness (I _really_ ought to be at least looking
around in the code before jumping in like this :-) ...
TBASE_DEFERRABLE_FLAG only {ab}uses the lowest bit of tvec_base_t *,
so clearly no pointer truncation issues here (you could still prefer
the unsigned long version for simplicity & style, though).

*embarrassed, goes for a cup of coffee*

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

[Jarek Poplawski]

On Wed, May 09, 2007 at 11:59:39PM +0530, Satyam Sharma wrote:
> 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:
...
> >>> >  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?

I am sorry, too - for my "logic". It seems it's all correct!
(Except, I don't know what's going here...)

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

	((unsigned int)(unsigned long)base ...
	((tvec_base_t *)((unsigned long)base ...
	((tvec_base_t *)((unsigned long)(timer->base) ...
	(tvec_base_t *)((unsigned long)(new_base) ...

Yes, if you don't count reading this one close each other, they are
clearly meaningles.

Regards,
Jarek P.