just for fun, i threw the following together to peruse the tree (or
any subdirectory) and look for stuff that violates the CodingStyle
guide. clearly, it's far from complete and very ad hoc, but it's
amusing. extra searches happily accepted.
rday
#!/bin/sh
DIR=${1-.}
echo -e "\n===== Deprecated 'depends' in Kconfig files ...\n"
grep depends $(find . -name Kconfig) | grep -v "depends on"
echo -e "\n===== Potentially unnecessary casting of k[cmz]alloc() calls ...\n"
grep -Er "\([^\)]+\) ?k[cmz]alloc ?\(" *
echo -e "\n===== Discouraged kcalloc(1,...) calls ...\n"
grep -Er "kcalloc ?\(1," *
echo -e "\n===== Where ARRAY_SIZE() should be used ...\n"
grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
echo -e "\n===== Where FIELD_SIZEOF() should be used ...\n"
grep -Er "sizeof ?\( ?\( ?\([^\*]*[^ ] ?\*\) ?0 ?\)->[^\)]+\)" *
echo -e "\n===== People who seem to be redefining MAX() or MIN() ...\n"
grep -Er "# +define +MAX ?\(" *
grep -Er "# +define +MIN ?\(" *
echo -e "\nDone."
On Tue, Dec 19, 2006 at 10:46:24AM -0500, Robert P. J. Day wrote:
>
> just for fun, i threw the following together to peruse the tree (or
> any subdirectory) and look for stuff that violates the CodingStyle
> guide. clearly, it's far from complete and very ad hoc, but it's
> amusing. extra searches happily accepted.
I had a bunch of similar greps that I've recently been half-assedly
putting together into a single script too.
See http://www.codemonkey.org.uk/projects/findbugs/
Dave
--
http://www.codemonkey.org.uk
On 12/19/06, Dave Jones <[email protected]> wrote:
> On Tue, Dec 19, 2006 at 10:46:24AM -0500, Robert P. J. Day wrote:
> >
> > just for fun, i threw the following together to peruse the tree (or
> > any subdirectory) and look for stuff that violates the CodingStyle
> > guide. clearly, it's far from complete and very ad hoc, but it's
> > amusing. extra searches happily accepted.
>
> I had a bunch of similar greps that I've recently been half-assedly
> putting together into a single script too.
> See http://www.codemonkey.org.uk/projects/findbugs/
I don't know if anyone cares about them anymore, since I think gcc
grew some smarts in the area recently, but there are a lot of lines of
code matching "static int.*= *0;" and equivalents in the driver tree.
Bob
>> > just for fun, i threw the following together to peruse the tree (or
>> > any subdirectory) and look for stuff that violates the CodingStyle
>> > guide. clearly, it's far from complete and very ad hoc, but it's
>> > amusing. extra searches happily accepted.
>>
>> I had a bunch of similar greps that I've recently been half-assedly
>> putting together into a single script too.
>> See http://www.codemonkey.org.uk/projects/findbugs/
>
> I don't know if anyone cares about them anymore, since I think gcc
> grew some smarts in the area recently, but there are a lot of lines of
> code matching "static int.*= *0;" and equivalents in the driver tree.
I'd really like to see the C compiler being enhanced to detect
"stupid casts", i.e. those, which when removed, do not change (a) the outcome
(b) the compiler warnings/error output.
-`J'
--
On Tue, 19 Dec 2006, Jan Engelhardt wrote:
> > I don't know if anyone cares about them anymore, since I think gcc
> > grew some smarts in the area recently, but there are a lot of lines of
> > code matching "static int.*= *0;" and equivalents in the driver tree.
>
> I'd really like to see the C compiler being enhanced to detect
> "stupid casts", i.e. those, which when removed, do not change (a) the outcome
> (b) the compiler warnings/error output.
>
If your desire is for the compiler warnings output to be unchanged, I'm
not sure how you'd enhance the compiler from detecting these casts. All
of the casts that have been removed in these cleanup patches do not change
the assembly when using gcc; they simply reduce the amount of visual noise
in the source code.
This is also true in terms of global static variables being initialized to
0 (or NULL). While it is indeed unnecessary by the standard, it simply
moves the initialization from one segment of the assembly to the other,
regardless of how many different functions it is referenced in. gcc does
not emit movl $0, var for these cases.
It _would_ be helpful to add a macro such as:
#define SILENCE_GCC(x) = x
to eliminate warnings such that:
auto int a SILENCE_GCC(a);
fill_a(&a);
if (a)
...
would not produce a "may be used uninitialized" warning.
David
On Dec 19 2006 13:24, David Rientjes wrote:
>On Tue, 19 Dec 2006, Jan Engelhardt wrote:
>
>> > I don't know if anyone cares about them anymore, since I think gcc
>> > grew some smarts in the area recently, but there are a lot of lines of
>> > code matching "static int.*= *0;" and equivalents in the driver tree.
>>
>> I'd really like to see the C compiler being enhanced to detect
>> "stupid casts", i.e. those, which when removed, do not change (a) the outcome
>> (b) the compiler warnings/error output.
>
>If your desire is for the compiler warnings output to be unchanged, I'm
Oh well maybe I suck at writing exact definitions in one-phrase phrases.
Anyway, what was intended:
(1) Catch casts where they are usually not necessary, because the value
would be anyhow discarded
(void)strcpy(b, a);
(Does not apply to e.g. cmpxchg -- that's why I mentioned (b)!)
(2) Catch casts which do not change the type of an expression, e.g.
void *x = (void *)kmalloc(...)
(3) Catch casts which do not change the outcome, because from-to-void* is
warningless
struct foo *bar = (struct foo *)kmalloc(...)
Quite often seen in kernel code.
(4) extension and truncation, implicit conversions - unneessary casts
func_taking_u32( (u32)some_u16 );
func_taking_u16( (u16)some_u32 );
(5) Slightly harder one: Where the evaluation process changes, but the
outcome is the same
#define V_FL_BASE_HI(x) ((x) << 8)
#define V_FL_INDEX_LO(x) (x)
#define M_FL_INDEX_LO 0xFF
static void t3_write_reg(struct adapter *, u32, u32);
int t3_sge_init_flcntxt(struct adapter *adapter, unsigned int id,
int gts_enable, u64 base_addr, unsigned int size,
unsigned int bsize, unsigned int cong_thres, int gen,
unsigned int cidx)
{
t3_write_reg(adapter, A_SG_CONTEXT_DATA1,
V_FL_BASE_HI((u32) base_addr) |
V_FL_INDEX_LO(cidx & M_FL_INDEX_LO));
}
(Note that this is not exactly code found in the cxgb3 driver, but
tweaked for this example)
As far as I can see, even if base_addr was not truncated to u32, the end
result would be, when the u64 value is passed to t3_write_reg.
>not sure how you'd enhance the compiler from detecting these casts. All
>of the casts that have been removed in these cleanup patches do not change
>the assembly when using gcc; they simply reduce the amount of visual noise
>in the source code.
>
>This is also true in terms of global static variables being initialized to
>0 (or NULL). While it is indeed unnecessary by the standard, it simply
>moves the initialization from one segment of the assembly to the other,
>regardless of how many different functions it is referenced in. gcc does
>not emit movl $0, var for these cases.
>
>It _would_ be helpful to add a macro such as:
>
> #define SILENCE_GCC(x) = x
>
>to eliminate warnings such that:
>
> auto int a SILENCE_GCC(a);
> fill_a(&a);
> if (a)
> ...
>
>would not produce a "may be used uninitialized" warning.
__attribute__((used)) would be more appropriate, I think.
-`J'
--
On Tue, 19 Dec 2006, Jan Engelhardt wrote:
> (1) Catch casts where they are usually not necessary, because the value
> would be anyhow discarded
>
> (void)strcpy(b, a);
>
> (Does not apply to e.g. cmpxchg -- that's why I mentioned (b)!)
>
Actually, there's a number of people out there who would like
(void)strcpy(b, a) or even (void)printf(...) to be required, but the gcc
manual addresses that point directly:
10.10 Certain Changes We Don't Want to Make
- Warning when a non-void function value is ignored
C contains many standard functions that return a value that
most programs choose to ignore. One obvious example is printf.
Warning about this practice only leads the defensive programmer
to clutter programs with dozens of casts to void. Such casts
are required so frequently that they become visual noise.
Writing these casts becomes so automatic that they no longer
convey useful information about the intentions of the
programmer. For functions where the return value should never
be ignored, use the warn_unused_result function attribute.
So there _is_ a way to manipulate the warning behavior of gcc for this
point.
> (2) Catch casts which do not change the type of an expression, e.g.
>
> void *x = (void *)kmalloc(...)
>
And do what with it? It's a no-op. And when there's tokens such as this
in source code that become no-ops on compile, it serves as an annotation
to the code reader that kmalloc returns void*.
> (3) Catch casts which do not change the outcome, because from-to-void* is
> warningless
>
> struct foo *bar = (struct foo *)kmalloc(...)
>
So you would favor some sort of gcc warning such as "unnecessary cast"
when this is compiled? If so, then there's a _tons_ of other warnings you
could generate when something exists in source code that does nothing.
Should we warn on every lone semi-colon that appears or every empty {}
block?
int var = (int)(int)(int)13;
is compiled _exactly_ the same as:
int var = 13;
> (4) extension and truncation, implicit conversions - unneessary casts
>
> func_taking_u32( (u32)some_u16 );
> func_taking_u16( (u16)some_u32 );
>
Again, these would be no-ops so the casts here actually don't do anything
and serve as _annotation_ for the code reader since functions aren't
typically named func_taking_u16. The compiler knows that the function
takes a u16 and can truncate the actual, but it serves to remind the
programmer that this value can be truncated later.
truncate(char ret)
{
return (int)ret;
}
main()
{
int var = 256;
int ret = test(var);
return ret;
}
This compiles without warning. So if there _was_ an "annotation" such as
int ret = test((char)var);
then the programmer is going to catch the bug much easier especially when
he doesn't check the API of the interface he's using first.
> (5) Slightly harder one: Where the evaluation process changes, but the
> outcome is the same
>
> #define V_FL_BASE_HI(x) ((x) << 8)
> #define V_FL_INDEX_LO(x) (x)
> #define M_FL_INDEX_LO 0xFF
>
> static void t3_write_reg(struct adapter *, u32, u32);
>
> int t3_sge_init_flcntxt(struct adapter *adapter, unsigned int id,
> int gts_enable, u64 base_addr, unsigned int size,
> unsigned int bsize, unsigned int cong_thres, int gen,
> unsigned int cidx)
> {
> t3_write_reg(adapter, A_SG_CONTEXT_DATA1,
> V_FL_BASE_HI((u32) base_addr) |
> V_FL_INDEX_LO(cidx & M_FL_INDEX_LO));
> }
>
> (Note that this is not exactly code found in the cxgb3 driver, but
> tweaked for this example)
>
> As far as I can see, even if base_addr was not truncated to u32, the end
> result would be, when the u64 value is passed to t3_write_reg.
>
You would get the same result regardless of whether the cast was there or
not. But, again, the u32 "cast" appears only as annotation so the
programmer is aware that the upper 32-bits of base_addr are going to be
discarded.
This is precisely why these lines emit the exact same assembly:
auto int var = (int)(char)256;
auto int var = (char)256;
Now, if you wrote:
auto char var = 256;
Then this would emit a warning because the programmer did not explictly
recognize 256 as overflow for a char.
> __attribute__((used)) would be more appropriate, I think.
>
As far as I know, you can't use this on automatic variables and
-Wuninitialized will still emit the warning message. It's great for the
static case.
David
Jan Engelhardt wrote:
>>>> just for fun, i threw the following together to peruse the tree (or
>>>> any subdirectory) and look for stuff that violates the CodingStyle
>>>> guide. clearly, it's far from complete and very ad hoc, but it's
>>>> amusing. extra searches happily accepted.
>>> I had a bunch of similar greps that I've recently been half-assedly
>>> putting together into a single script too.
>>> See http://www.codemonkey.org.uk/projects/findbugs/
>> I don't know if anyone cares about them anymore, since I think gcc
>> grew some smarts in the area recently, but there are a lot of lines of
>> code matching "static int.*= *0;" and equivalents in the driver tree.
>
> I'd really like to see the C compiler being enhanced to detect
> "stupid casts", i.e. those, which when removed, do not change (a) the outcome
> (b) the compiler warnings/error output.
Bearing in mind that some casts may have been put in when struct members
had other values, may be needed on some hardware but not others, etc.
Cleanups are good, but may not be as obvious as they appear.
Not that there's a lack of places to remove visual cruft, but perhaps
someone could look at casts and ask if each hides a real type mismatch.
--
bill davidsen <[email protected]>
CTO TMR Associates, Inc
Doing interesting things with small computers since 1979
On Dec 20 2006 17:42, Bill Davidsen wrote:
>
> Bearing in mind that some casts may have been put in when struct
> members had other values, may be needed on some hardware but not
> others, etc. Cleanups are good, but may not be as obvious as they
> appear.
>
> Not that there's a lack of places to remove visual cruft, but
> perhaps someone could look at casts and ask if each hides a real
> type mismatch.
http://lkml.org/lkml/2006/9/30/208
As much as I would like to go through the whole kernel tree, it's a
task quite big.
-`J'
--
On Dec 21 2006 14:53, Joe Perches wrote:
>On Thu, 2006-12-21 at 21:52 +0100, Jan Engelhardt wrote:
>> http://lkml.org/lkml/2006/9/30/208
>
>@@ -1302,7 +1302,7 @@ static int acpi_battery_add(struct acpi_
> battery->init_state = 1;
> }
>
>- (void)sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
>+ sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
>
>These casts can eliminate "return value unused" warnings.
But only when functions are tagged __must_check, and sprintf is not.
cmpxchg is one where (void) is 'needed', __as I wrote__ in a cxgb3
comment.
After applying this patch, there are no additional warnings:
00:19 ichi:/erk/kernel/linux-2.6.20-rc1 > make drivers/acpi/sbs.o
CHK include/linux/version.h
CHK include/linux/utsrelease.h
CC [M] drivers/acpi/sbs.o
00:21 ichi:/erk/kernel/linux-2.6.20-rc1 > grep MUST .config
CONFIG_ENABLE_MUST_CHECK=y
akpm, please include.
---
Remove some unnecessary (void) casts.
Signed-off-by: Jan Engelhardt <[email protected]>
Index: linux-2.6.20-rc1/drivers/acpi/sbs.c
===================================================================
--- linux-2.6.20-rc1.orig/drivers/acpi/sbs.c
+++ linux-2.6.20-rc1/drivers/acpi/sbs.c
@@ -1160,14 +1160,14 @@ acpi_battery_write_alarm(struct file *fi
if (result) {
ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
"acpi_battery_set_alarm() failed\n"));
- (void)acpi_battery_set_alarm(battery, old_alarm);
+ acpi_battery_set_alarm(battery, old_alarm);
goto end;
}
result = acpi_battery_get_alarm(battery);
if (result) {
ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
"acpi_battery_get_alarm() failed\n"));
- (void)acpi_battery_set_alarm(battery, old_alarm);
+ acpi_battery_set_alarm(battery, old_alarm);
goto end;
}
@@ -1302,7 +1302,7 @@ static int acpi_battery_add(struct acpi_
battery->init_state = 1;
}
- (void)sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
+ sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
result = acpi_sbs_generic_add_fs(&battery->battery_entry,
acpi_battery_dir,
@@ -1485,7 +1485,7 @@ static int acpi_sbs_update_run(struct ac
}
if (old_battery_present != new_battery_present) {
- (void)sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
+ sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
result = acpi_sbs_generate_event(sbs->device,
ACPI_SBS_BATTERY_NOTIFY_STATUS,
new_battery_present,
@@ -1498,7 +1498,7 @@ static int acpi_sbs_update_run(struct ac
}
}
if (old_remaining_capacity != battery->state.remaining_capacity) {
- (void)sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
+ sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
result = acpi_sbs_generate_event(sbs->device,
ACPI_SBS_BATTERY_NOTIFY_STATUS,
new_battery_present,
On Dec 21 2006 15:40, Joe Perches wrote:
>On Fri, 2006-12-22 at 00:29 +0100, Jan Engelhardt wrote:
>> On Dec 21 2006 14:53, Joe Perches wrote:
>> >On Thu, 2006-12-21 at 21:52 +0100, Jan Engelhardt wrote:
>> >> http://lkml.org/lkml/2006/9/30/208
>> >@@ -1302,7 +1302,7 @@ static int acpi_battery_add(struct acpi_
>> > battery->init_state = 1;
>> > }
>> >- (void)sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
>> >+ sprintf(dir_name, ACPI_BATTERY_DIR_NAME, id);
>> >These casts can eliminate "return value unused" warnings.
>>
>> But only when functions are tagged __must_check, and sprintf is not.
>
>or where -Wall is used.
00:50 takeshi:/dev/shm > cat bla.c
#include <stdio.h>
int main(void) {
char foo[42];
sprintf(foo, "bar");
return 0;
}
00:52 takeshi:/dev/shm > cc bla.c -Wall
(no warnings)
In case you were talking about kernel code, the same (i.e. no warnings)
happens:
00:54 takeshi:/dev/shm > make -C /lib/modules/2.6.18.5-jen40b-default/build M=$PWD
make: Entering directory `/usr/src/linux-2.6.18.5-jen40b-obj/i386/default'
make -C ../../../linux-2.6.18.5-jen40b O=../linux-2.6.18.5-jen40b-obj/i386/default
CC [M] /dev/shm/bla2.o
Building modules, stage 2.
MODPOST
LD [M] /dev/shm/bla2.ko
make: Leaving directory `/usr/src/linux-2.6.18.5-jen40b-obj/i386/default'
00:54 takeshi:/dev/shm > cat Makefile
EXTRA_CFLAGS += -Wall
obj-m += bla2.o
-`J'
--
On Fri, 22 Dec 2006, Jan Engelhardt wrote:
> >These casts can eliminate "return value unused" warnings.
>
> But only when functions are tagged __must_check, and sprintf is not.
> cmpxchg is one where (void) is 'needed', __as I wrote__ in a cxgb3
> comment.
>
gcc requires functions to be declared with the attribute
warn_unused_result if a warning should be emitted in these cases. So
casting sprintf or any function without warn_unused_result to (void) is
only visual noise within the source code. Thus, the patch is correct.
David
Jan Engelhardt wrote:
>>>> just for fun, i threw the following together to peruse the tree (or
>>>> any subdirectory) and look for stuff that violates the CodingStyle
>>>> guide. clearly, it's far from complete and very ad hoc, but it's
>>>> amusing. extra searches happily accepted.
>>> I had a bunch of similar greps that I've recently been half-assedly
>>> putting together into a single script too.
>>> See http://www.codemonkey.org.uk/projects/findbugs/
>> I don't know if anyone cares about them anymore, since I think gcc
>> grew some smarts in the area recently, but there are a lot of lines of
>> code matching "static int.*= *0;" and equivalents in the driver tree.
>
> I'd really like to see the C compiler being enhanced to detect
> "stupid casts", i.e. those, which when removed, do not change (a) the outcome
> (b) the compiler warnings/error output.
>
If you made it issue warnings for that, then it's going to be ever more
painful to write generic macros.
-hpa