Messages in this thread | | | Date | Thu, 10 May 2007 07:52:57 +0200 | From | Jarek Poplawski <> | Subject | Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc. |
| |
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |