Before:
total: 5 errors, 0 warnings, 48 lines checked
After:
total: 0 errors, 0 warnings, 58 lines checked
paolo@paolo-desktop:~/linux.trees.git$ md5sum /tmp/bios_uv.o.*
9afe794594831166704744184e192ed8 /tmp/bios_uv.o.after
9afe794594831166704744184e192ed8 /tmp/bios_uv.o.before
Signed-off-by: Paolo Ciarrocchi <[email protected]>
---
arch/x86/kernel/bios_uv.c | 20 +++++++++++++++-----
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/bios_uv.c b/arch/x86/kernel/bios_uv.c
index c639bd5..100e759 100644
--- a/arch/x86/kernel/bios_uv.c
+++ b/arch/x86/kernel/bios_uv.c
@@ -25,11 +25,21 @@ x86_bios_strerror(long status)
{
const char *str;
switch (status) {
- case 0: str = "Call completed without error"; break;
- case -1: str = "Not implemented"; break;
- case -2: str = "Invalid argument"; break;
- case -3: str = "Call completed with error"; break;
- default: str = "Unknown BIOS status code"; break;
+ case 0:
+ str = "Call completed without error";
+ break;
+ case -1:
+ str = "Not implemented";
+ break;
+ case -2:
+ str = "Invalid argument";
+ break;
+ case -3:
+ str = "Call completed with error";
+ break;
+ default:
+ str = "Unknown BIOS status code";
+ break;
}
return str;
}
--
1.5.6.rc1.21.g03300
Paolo Ciarrocchi wrote:
>
> diff --git a/arch/x86/kernel/bios_uv.c b/arch/x86/kernel/bios_uv.c
> index c639bd5..100e759 100644
> --- a/arch/x86/kernel/bios_uv.c
> +++ b/arch/x86/kernel/bios_uv.c
> @@ -25,11 +25,21 @@ x86_bios_strerror(long status)
> {
> const char *str;
> switch (status) {
> - case 0: str = "Call completed without error"; break;
> - case -1: str = "Not implemented"; break;
> - case -2: str = "Invalid argument"; break;
> - case -3: str = "Call completed with error"; break;
> - default: str = "Unknown BIOS status code"; break;
> + case 0:
> + str = "Call completed without error";
> + break;
> + case -1:
> + str = "Not implemented";
> + break;
> + case -2:
> + str = "Invalid argument";
> + break;
> + case -3:
> + str = "Call completed with error";
> + break;
> + default:
> + str = "Unknown BIOS status code";
> + break;
> }
> return str;
> }
This should be an array in the first place...
-hpa
H. Peter Anvin wrote:
> Paolo Ciarrocchi wrote:
>>
>> diff --git a/arch/x86/kernel/bios_uv.c b/arch/x86/kernel/bios_uv.c
>> index c639bd5..100e759 100644
>> --- a/arch/x86/kernel/bios_uv.c
>> +++ b/arch/x86/kernel/bios_uv.c
>> @@ -25,11 +25,21 @@ x86_bios_strerror(long status)
>> {
>> const char *str;
>> switch (status) {
>> - case 0: str = "Call completed without error"; break;
>> - case -1: str = "Not implemented"; break;
>> - case -2: str = "Invalid argument"; break;
>> - case -3: str = "Call completed with error"; break;
>> - default: str = "Unknown BIOS status code"; break;
>> + case 0:
>> + str = "Call completed without error";
>> + break;
>> + case -1:
>> + str = "Not implemented";
>> + break;
>> + case -2:
>> + str = "Invalid argument";
>> + break;
>> + case -3:
>> + str = "Call completed with error";
>> + break;
>> + default:
>> + str = "Unknown BIOS status code";
>> + break;
>> }
>> return str;
>> }
>
> This should be an array in the first place...
Besides, by following CodingStyle to the letter, it arguably breaks
rather than fixes coding style. The former code was easy enough to read.
--
Stefan Richter
-=====-==--- =--- ---==
http://arcgraph.de/sr/
Stefan Richter <[email protected]> writes:
>>> const char *str;
>>> switch (status) {
>>> - case 0: str = "Call completed without error"; break;
>>> - case -1: str = "Not implemented"; break;
>>> - case -2: str = "Invalid argument"; break;
>>> - case -3: str = "Call completed with error"; break;
>>> - default: str = "Unknown BIOS status code"; break;
>>> + case 0:
>>> + str = "Call completed without error";
>>> + break;
>>> + case -1:
>>> + str = "Not implemented";
>>> + break;
>>> + case -2:
>>> + str = "Invalid argument";
>>> + break;
>>> + case -3:
>>> + str = "Call completed with error";
>>> + break;
>>> + default:
>>> + str = "Unknown BIOS status code";
>>> + break;
>>> }
>>> return str;
>>> }
>>
>> This should be an array in the first place...
>
> Besides, by following CodingStyle to the letter, it arguably breaks
> rather than fixes coding style. The former code was easy enough to
> read.
Right. The latter is much worse. That's why nobody can trust
CodingStyle and/or checkpatch automatically. It's only sane mode
of operation is as a help tool for authors and maintainers, to
quickly locate _potential_ problems.
--
Krzysztof Halasa
On Sun, Aug 3, 2008 at 1:54 PM, Krzysztof Halasa <[email protected]> wrote:
[...]
> Right. The latter is much worse. That's why nobody can trust
> CodingStyle and/or checkpatch automatically. It's only sane mode
> of operation is as a help tool for authors and maintainers, to
> quickly locate _potential_ problems.
_much worse_? Wow, I thought the opposite.
Anyway, that's just a coding style patch, feel free to simply not apply it as
it's really a matter of personal taste but be sure that I'm not
blindly changing the code
accordingly to checkpatch. That might be almost true when I started
doing this kind of patches months
ago but it's definitely not true anymore.
Thanks.
regards,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
Paolo Ciarrocchi wrote:
>
> _much worse_? Wow, I thought the opposite.
> Anyway, that's just a coding style patch, feel free to simply not apply it as
> it's really a matter of personal taste but be sure that I'm not
> blindly changing the code
> accordingly to checkpatch. That might be almost true when I started
> doing this kind of patches months
> ago but it's definitely not true anymore.
>
In this case, the real problem is that the original code used a switch
statement where it really should have used a table.
-hpa
* Paolo Ciarrocchi <[email protected]> wrote:
> Before:
> total: 5 errors, 0 warnings, 48 lines checked
>
> After:
> total: 0 errors, 0 warnings, 58 lines checked
>
> paolo@paolo-desktop:~/linux.trees.git$ md5sum /tmp/bios_uv.o.*
> 9afe794594831166704744184e192ed8 /tmp/bios_uv.o.after
> 9afe794594831166704744184e192ed8 /tmp/bios_uv.o.before
>
> Signed-off-by: Paolo Ciarrocchi <[email protected]>
> ---
> arch/x86/kernel/bios_uv.c | 20 +++++++++++++++-----
> 1 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/bios_uv.c b/arch/x86/kernel/bios_uv.c
> index c639bd5..100e759 100644
> --- a/arch/x86/kernel/bios_uv.c
> +++ b/arch/x86/kernel/bios_uv.c
> @@ -25,11 +25,21 @@ x86_bios_strerror(long status)
> {
> const char *str;
> switch (status) {
> - case 0: str = "Call completed without error"; break;
> - case -1: str = "Not implemented"; break;
> - case -2: str = "Invalid argument"; break;
> - case -3: str = "Call completed with error"; break;
> - default: str = "Unknown BIOS status code"; break;
> + case 0:
> + str = "Call completed without error";
> + break;
> + case -1:
> + str = "Not implemented";
> + break;
> + case -2:
> + str = "Invalid argument";
> + break;
> + case -3:
> + str = "Call completed with error";
> + break;
> + default:
> + str = "Unknown BIOS status code";
> + break;
hm - i changed your patch to the one below, to align the break's
vertically, which makes the original variant a lot more readable (and
even more readable than the new one). Checkpatch still complains about
it but that's OK, there are always exceptions.
Ingo
------------------->
Subject: x86: coding style fixes to arch/x86/kernel/bios_uv.c
From: Paolo Ciarrocchi <[email protected]>
Date: Sat, 2 Aug 2008 21:24:06 +0200
paolo@paolo-desktop:~/linux.trees.git$ md5sum /tmp/bios_uv.o.*
9afe794594831166704744184e192ed8 /tmp/bios_uv.o.after
9afe794594831166704744184e192ed8 /tmp/bios_uv.o.before
Signed-off-by: Paolo Ciarrocchi <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/bios_uv.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Index: tip/arch/x86/kernel/bios_uv.c
===================================================================
--- tip.orig/arch/x86/kernel/bios_uv.c
+++ tip/arch/x86/kernel/bios_uv.c
@@ -25,11 +25,11 @@ x86_bios_strerror(long status)
{
const char *str;
switch (status) {
- case 0: str = "Call completed without error"; break;
- case -1: str = "Not implemented"; break;
- case -2: str = "Invalid argument"; break;
- case -3: str = "Call completed with error"; break;
- default: str = "Unknown BIOS status code"; break;
+ case 0: str = "Call completed without error"; break;
+ case -1: str = "Not implemented"; break;
+ case -2: str = "Invalid argument"; break;
+ case -3: str = "Call completed with error"; break;
+ default: str = "Unknown BIOS status code"; break;
}
return str;
}
On 8/15/08, Ingo Molnar <[email protected]> wrote:
> hm - i changed your patch to the one below, to align the break's
> vertically, which makes the original variant a lot more readable (and
> even more readable than the new one). Checkpatch still complains about
> it but that's OK, there are always exceptions.
>
thank you Ingo.
ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/