Re: my handy-dandy, "coding style" script
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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 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' -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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_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, - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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' -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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' -- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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, - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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' -- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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_LO0xFF > > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at
Re: my handy-dandy, "coding style" script
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_LO0xFF 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(); > if (a) > ... > >would not produce a "may be used uninitialized" warning. __attribute__((used)) would be more appropriate, I think. -`J' -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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(); if (a) ... would not produce a "may be used uninitialized" warning. David - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
>> > 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' -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, "coding style" script
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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' -- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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_LO0xFF 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' -- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: my handy-dandy, coding style script
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_LO0xFF 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ