my handy-dandy, "coding style" script

my handy-dandy, "coding style" script

[Robert P. J. Day]

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

Re: my handy-dandy, "coding style" script

[Dave Jones]

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

Re: my handy-dandy, "coding style" script

[Bob Copeland]

On 12/19/06, Dave Jones <davej@redhat.com> 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

Re: my handy-dandy, "coding style" script

[Jan Engelhardt]

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

Re: my handy-dandy, "coding style" script

[David Rientjes]

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

Re: my handy-dandy, "coding style" script

[Jan Engelhardt]

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

Re: my handy-dandy, "coding style" script

[David Rientjes]

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

Re: my handy-dandy, "coding style" script

[Bill Davidsen]

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 <davidsen@tmr.com>
   CTO TMR Associates, Inc
   Doing interesting things with small computers since 1979

Re: my handy-dandy, "coding style" script

[Jan Engelhardt]

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

Re: my handy-dandy, "coding style" script

[Jan Engelhardt]

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 <jengelh@gmx.de>

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,

Re: my handy-dandy, "coding style" script

[Jan Engelhardt]

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

Re: my handy-dandy, "coding style" script

[David Rientjes]

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

Re: my handy-dandy, "coding style" script

[H. Peter Anvin]

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