timer: parenthesis fix in tbase_get_deferrable() etc.

PATCH In review Jarek Poplawski 4 participants 13 messages May 8, 2007 – May 10, 2007

lore

Share & tools

lore

Jarek Poplawski

author

2007-05-08 10:27 UTC #0 lore raw

PATCH 1/1 timer: parenthesis fix in tbase_get_deferrable() etc.

Signed-off-by Jarek Poplawski <[email protected]>

---

kernel/timer.c +5 -5

diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
@@ -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));
 }

Andrew Morton

2007-05-08 23:34 UTC #1 lore raw

Quoted 28 lines · On Tue, 8 May 2007 12:33:48 +0200 Jarek Poplawski <[email protected]> wrote:

Signed-off-by Jarek Poplawski <[email protected]>

---

diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c

kernel/timer.c +4 -4

@@ -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);

kernel/timer.c static DEFINE_PER_CPU(tvec_base_t *, tve · line 115

Quoted 10 lines

kernel/timer.c +1 -1

 }
 
 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?

Jarek Poplawski

author

2007-05-09 05:25 UTC #2 lore raw

Quoted 18 lines · Andrew Morton

Quoted 15 lines · On Tue, 8 May 2007 12:33:48 +0200 Jarek Poplawski <[email protected]> wrote:
Signed-off-by Jarek Poplawski <[email protected]>
---
diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
kernel/timer.c +1 -1
@@ -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);
 }

...

Quoted 2 lines

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.

Pallipadi, Venkatesh

2007-05-09 15:59 UTC #3 lore raw

Quoted 31 lines

-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Tuesday, May 08, 2007 10:32 PM
To: Andrew Morton
Cc: Pallipadi, Venkatesh; [email protected]; Oleg Nesterov
Subject: Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc.
Quoted 10 lines · Andrew Morton
Quoted 7 lines · On Tue, 8 May 2007 12:33:48 +0200 Jarek Poplawski <[email protected]> wrote:
Signed-off-by Jarek Poplawski <[email protected]>
---
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
Quoted 1 line
Quoted 1 line
+++ 2.6.21-mm1/kernel/timer.c 2007-05-08
12:05:11.000000000 +0200
Quoted 5 lines
kernel/timer.c static DEFINE_PER_CPU(tvec_base_t *, tve · line 95
Quoted 5 lines
kernel/timer.c +0 -1
@@ -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);
Quoted 1 line
Review static DEFINE_PER_CPU(tvec_base_t *, tve · line 95
Quoted 1 line
(quoted diff) +1 -0
+	return (unsigned int)((unsigned long)base & 
TBASE_DEFERRABLE_FLAG);

Quoted 9 lines

Quoted 1 line
Quoted 1 line
(quoted diff) +0 -0
 }
...
Quoted 2 lines
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

Satyam Sharma

2007-05-09 18:29 UTC #4 lore raw

Quoted 41 lines · Venkatesh

Quoted 31 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Tuesday, May 08, 2007 10:32 PM
To: Andrew Morton
Cc: Pallipadi, Venkatesh; [email protected]; Oleg Nesterov
Subject: Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc.
Quoted 10 lines · Andrew Morton
Quoted 7 lines · On Tue, 8 May 2007 12:33:48 +0200 Jarek Poplawski <[email protected]> wrote:
Signed-off-by Jarek Poplawski <[email protected]>
---
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
Quoted 1 line
Quoted 1 line
+++ 2.6.21-mm1/kernel/timer.c 2007-05-08
12:05:11.000000000 +0200
Quoted 5 lines
kernel/timer.c static DEFINE_PER_CPU(tvec_base_t *, tve · line 95
Quoted 5 lines
kernel/timer.c +0 -1
@@ -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);
Quoted 1 line
Review static DEFINE_PER_CPU(tvec_base_t *, tve · line 95
Quoted 1 line
(quoted diff) +1 -0
+  return (unsigned int)((unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 9 lines
Quoted 1 line
Quoted 1 line
(quoted diff) +0 -0
 }
...
Quoted 2 lines
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.)

Quoted 7 lines

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.

Pallipadi, Venkatesh

2007-05-09 19:10 UTC #5 lore raw

Quoted 68 lines

-----Original Message-----
From: Satyam Sharma [mailto:[email protected]]
Sent: Wednesday, May 09, 2007 11:30 AM
To: Pallipadi, Venkatesh
Cc: Jarek Poplawski; Andrew Morton; [email protected]; Oleg Nesterov
Subject: Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc.
Quoted 6 lines · Venkatesh
Quoted 6 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Tuesday, May 08, 2007 10:32 PM
To: Andrew Morton
Cc: Pallipadi, Venkatesh; [email protected];
Oleg Nesterov
Quoted 12 lines
Quoted 12 lines
Subject: Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc.
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Tue, 8 May 2007 12:33:48 +0200 Jarek Poplawski <[email protected]> wrote:
Signed-off-by Jarek Poplawski <[email protected]>
---
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 1 line
Quoted 1 line
+++ 2.6.21-mm1/kernel/timer.c 2007-05-08
12:05:11.000000000 +0200
Quoted 3 lines
Review static DEFINE_PER_CPU(tvec_base_t *, tve · line 93
Quoted 3 lines
(quoted diff) +0 -0
@@ -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)
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
Review static DEFINE_PER_CPU(tvec_base_t *, tve · line 95
Quoted 2 lines
(quoted diff) +0 -1
 {
-  return ((unsigned int)(unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 1 line
Review static DEFINE_PER_CPU(tvec_base_t *, tve · line 95
Quoted 1 line
(quoted diff) +1 -0
+  return (unsigned int)((unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 1 line
Quoted 1 line
(quoted diff) +0 -0
 }
...
Quoted 2 lines
The change makes sense, but does it actually "fix" anything?
Quoted 6 lines
Quoted 4 lines
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
Quoted 6 lines
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.

Quoted 7 lines

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

Jarek Poplawski

author

2007-05-10 07:32 UTC #6 lore raw

On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
...

Quoted 6 lines

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.

Andrew Morton

2007-05-10 07:57 UTC #7 lore raw

Quoted 9 lines · On Thu, 10 May 2007 09:39:04 +0200 Jarek Poplawski <[email protected]> wrote:

On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
...
Quoted 6 lines
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.

Quoted 4 lines

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.

Jarek Poplawski

author

2007-05-10 09:23 UTC #8 lore raw

PATCH 1/1 timer: revert parenthesis fix in tbase_get_deferrable() etc.

Quoted 11 lines · Andrew Morton

Quoted 9 lines · On Thu, 10 May 2007 09:39:04 +0200 Jarek Poplawski <[email protected]> wrote:
On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
...
Quoted 6 lines
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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 8 lines

Quoted 8 lines · Venkatesh
...
Quoted 6 lines
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" <[email protected]>

Cc Satyam Sharma <[email protected]>

Cc Oleg Nesterov <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

kernel/timer.c +5 -5

diff -Nurp 2.6.21-git12-/kernel/timer.c 2.6.21-git12/kernel/timer.c
@@ -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));
 }

Jarek Poplawski

author

2007-05-10 09:29 UTC #9 lore raw

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

On Thu, May 10, 2007 at 11:29:47AM +0200, Jarek Poplawski wrote:
...

Quoted 11 lines

[PATCH] timer: revert parenthesis fix in tbase_get_deferrable() etc.
Quoted 8 lines
Quoted 8 lines · Venkatesh
...
Quoted 6 lines
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.

Quoted 3 lines

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

Satyam Sharma

2007-05-10 08:27 UTC #10 lore raw

In reply to Pallipadi, Venkatesh

Quoted 38 lines

Quoted 30 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
static inline unsigned int
tbase_get_deferrable(tvec_base_t *base)
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
Quoted 2 lines
{
- return ((unsigned int)(unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 1 line
Quoted 1 line
+ return (unsigned int)((unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 1 line
Quoted 1 line
}
...
Quoted 2 lines
The change makes sense, but does it actually "fix" anything?
Quoted 6 lines
Quoted 4 lines
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
Quoted 6 lines
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.
Quoted 7 lines
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.

Quoted 3 lines

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.

Satyam Sharma

2007-05-10 09:32 UTC #11 lore raw

Quoted 15 lines · Satyam Sharma

Quoted 4 lines
[...]
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);
}

Quoted 6 lines

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*

Jarek Poplawski

author

2007-05-10 05:46 UTC #12 lore raw

In reply to Satyam Sharma

Quoted 13 lines · Satyam Sharma

Quoted 12 lines · Venkatesh
Quoted 12 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Tuesday, May 08, 2007 10:32 PM
To: Andrew Morton
Cc: Pallipadi, Venkatesh; [email protected]; Oleg Nesterov
Subject: Re: [PATCH -mm] timer: parenthesis fix in tbase_get_deferrable() etc.
Quoted 2 lines · Andrew Morton
On Tue, 8 May 2007 12:33:48 +0200
Jarek Poplawski <[email protected]> wrote:

...

Quoted 11 lines

Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
Quoted 3 lines
static inline unsigned int tbase_get_deferrable(tvec_base_t *base) {
- return ((unsigned int)(unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 1 line
Quoted 1 line
+ return (unsigned int)((unsigned long)base &
TBASE_DEFERRABLE_FLAG);
Quoted 1 line
Quoted 1 line
}
...
Quoted 2 lines
The change makes sense, but does it actually "fix" anything?

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
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...)

Quoted 18 lines

Quoted 5 lines
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.

Quoted 2 lines

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.