Re: [PATCH] remove some usesless casts
On Thu, 21 April 2005 09:36:18 +0300, Pekka Enberg wrote: > > I think Jörn means that if you need an opaque data type, use void > pointers (which are automatically cast to the proper type) and that > all other casts are a design smell (except for the one or two special > cases where you actually need them). Two of my patches agree with you, two don't. Really, in almost all cases, casts are a Bad Idea(tm). Almost always, there is _something_ better. In some cases, this comes down to void pointers, yes. Jörn -- I can say that I spend most of my time fixing bugs even if I have lots of new features to implement in mind, but I give bugs more priority. -- Andrea Arcangeli, 2000 - 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: [PATCH] remove some usesless casts
Phillip, Jörn Engel wrote: > > Your definition of _unnecessary_ casts may differ from mine. > > Basically, every cast is unnecessary, except for maybe one or two - if > > that many. On 4/20/05, Phillip Lougher <[EMAIL PROTECTED]> wrote: > Well we agree to differ then. In my experience casts are sometimes > necessary, and are often less clumsy than the alternatives (such as > unions). This is probably a generational thing, the fashion today is to > make languages much more strongly typechecked than before. I think Jörn means that if you need an opaque data type, use void pointers (which are automatically cast to the proper type) and that all other casts are a design smell (except for the one or two special cases where you actually need them). Pekka - 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: [PATCH] remove some usesless casts
Phillip, Jörn Engel wrote: Your definition of _unnecessary_ casts may differ from mine. Basically, every cast is unnecessary, except for maybe one or two - if that many. On 4/20/05, Phillip Lougher [EMAIL PROTECTED] wrote: Well we agree to differ then. In my experience casts are sometimes necessary, and are often less clumsy than the alternatives (such as unions). This is probably a generational thing, the fashion today is to make languages much more strongly typechecked than before. I think Jörn means that if you need an opaque data type, use void pointers (which are automatically cast to the proper type) and that all other casts are a design smell (except for the one or two special cases where you actually need them). Pekka - 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: [PATCH] remove some usesless casts
On Thu, 21 April 2005 09:36:18 +0300, Pekka Enberg wrote: I think Jörn means that if you need an opaque data type, use void pointers (which are automatically cast to the proper type) and that all other casts are a design smell (except for the one or two special cases where you actually need them). Two of my patches agree with you, two don't. Really, in almost all cases, casts are a Bad Idea(tm). Almost always, there is _something_ better. In some cases, this comes down to void pointers, yes. Jörn -- I can say that I spend most of my time fixing bugs even if I have lots of new features to implement in mind, but I give bugs more priority. -- Andrea Arcangeli, 2000 - 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: [PATCH] remove some usesless casts
On Wed, 20 April 2005 21:51:15 +0100, Phillip Lougher wrote: > Jörn Engel wrote: > > >Your definition of _unnecessary_ casts may differ from mine. > >Basically, every cast is unnecessary, except for maybe one or two - if > >that many. > > Well we agree to differ then. In my experience casts are sometimes > necessary, and are often less clumsy than the alternatives (such as > unions). This is probably a generational thing, the fashion today is to > make languages much more strongly typechecked than before. I never claimed to replace the casts with unions. ;) Jörn -- Homo Sapiens is a goal, not a description. -- unknown - 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: [PATCH] remove some usesless casts
Jörn Engel wrote: Your definition of _unnecessary_ casts may differ from mine. Basically, every cast is unnecessary, except for maybe one or two - if that many. Well we agree to differ then. In my experience casts are sometimes necessary, and are often less clumsy than the alternatives (such as unions). This is probably a generational thing, the fashion today is to make languages much more strongly typechecked than before. - 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: [PATCH] remove some usesless casts
On Wed, 20 April 2005 16:20:10 +0100, Phillip Lougher wrote: > Jörn Engel wrote: > >Squashfs is extremely cast-happy. This patch makes it less so. > > Thanks for the patch. Unnecessary casts were one of the things > mentioned when I submitted the patches to the LKML, and therefore I > suspect most of them have been already fixed (but I will apply your > patch to check). Your definition of _unnecessary_ casts may differ from mine. Basically, every cast is unnecessary, except for maybe one or two - if that many. > I will send revised patches to the LKML soon, most of the issues raised > by the comments have been fixed, the current delay is being caused by > the 4GB limit re-work. There are three more patches in my queue, which I'll send after first coffee or so. After those I'll ignore squashfs for a while (until you send current code or so). Jörn -- The story so far: In the beginning the Universe was created. This has made a lot of people very angry and been widely regarded as a bad move. -- Douglas Adams - 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: [PATCH] remove some usesless casts
Jörn Engel wrote: Squashfs is extremely cast-happy. This patch makes it less so. Jörn Hi, Thanks for the patch. Unnecessary casts were one of the things mentioned when I submitted the patches to the LKML, and therefore I suspect most of them have been already fixed (but I will apply your patch to check). I will send revised patches to the LKML soon, most of the issues raised by the comments have been fixed, the current delay is being caused by the 4GB limit re-work. Regards Phillip - 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/
[PATCH] remove some usesless casts
Squashfs is extremely cast-happy. This patch makes it less so. Jörn -- If you're willing to restrict the flexibility of your approach, you can almost always do something better. -- John Carmack Signed-off-by: Jörn Engel <[EMAIL PROTECTED]> --- fs/squashfs/inode.c | 63 ++-- 1 files changed, 27 insertions(+), 36 deletions(-) --- linux-2.6.12-rc2cow/fs/squashfs/inode.c~squashfs_cu12005-04-20 07:52:46.0 +0200 +++ linux-2.6.12-rc2cow/fs/squashfs/inode.c 2005-04-20 08:11:10.254367656 +0200 @@ -111,7 +111,7 @@ struct inode_operations squashfs_dir_ino static struct buffer_head *get_block_length(struct super_block *s, int *cur_index, int *offset, int *c_byte) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; unsigned short temp; struct buffer_head *bh; @@ -176,7 +176,7 @@ unsigned int squashfs_read_data(struct s unsigned int index, unsigned int length, unsigned int *next_index) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; struct buffer_head *bh[((SQUASHFS_FILE_MAX_SIZE - 1) >> msBlk->devblksize_log2) + 2]; unsigned int offset = index & ((1 << msBlk->devblksize_log2) - 1); @@ -285,7 +285,7 @@ int squashfs_get_cached_block(struct sup int length, unsigned int *next_block, unsigned int *next_offset) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; int n, i, bytes, return_length = length; unsigned int next_index; @@ -390,7 +390,7 @@ static int get_fragment_location(struct unsigned int *fragment_start_block, unsigned int *fragment_size) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; unsigned int start_block = msBlk->fragment_index[SQUASHFS_FRAGMENT_INDEX(fragment)]; int offset = SQUASHFS_FRAGMENT_INDEX_OFFSET(fragment); @@ -434,7 +434,7 @@ static struct squashfs_fragment_cache *g int length) { int i, n; - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; for (;;) { down(>fragment_mutex); @@ -466,8 +466,7 @@ static struct squashfs_fragment_cache *g SQUASHFS_CACHED_FRAGMENTS; if (msBlk->fragment[i].data == NULL) - if (!(msBlk->fragment[i].data = - (unsigned char *) kmalloc + if (!(msBlk->fragment[i].data = kmalloc (SQUASHFS_FILE_MAX_SIZE, GFP_KERNEL))) { ERROR("Failed to allocate fragment " @@ -509,7 +508,7 @@ static struct squashfs_fragment_cache *g static struct inode *squashfs_new_inode(struct super_block *s, squashfs_base_inode_header *inodeb, unsigned int ino) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; squashfs_super_block *sBlk = >sBlk; struct inode *i = new_inode(s); @@ -535,7 +534,7 @@ static struct inode *squashfs_new_inode( static struct inode *squashfs_iget(struct super_block *s, squashfs_inode inode) { struct inode *i; - squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info; + squashfs_sb_info *msBlk = s->s_fs_info; squashfs_super_block *sBlk = >sBlk; unsigned int block = SQUASHFS_INODE_BLK(inode) + sBlk->inode_table_start; @@ -837,13 +836,12 @@ static int squashfs_fill_super(struct su TRACE("Entered squashfs_read_superblock\n"); - if (!(s->s_fs_info = (void *) kmalloc(sizeof(squashfs_sb_info), - GFP_KERNEL))) { + if (!(s->s_fs_info = kmalloc(sizeof(squashfs_sb_info), GFP_KERNEL))) { ERROR("Failed to allocate superblock\n"); return -ENOMEM; } memset(s->s_fs_info, 0, sizeof(squashfs_sb_info)); - msBlk = (squashfs_sb_info *) s->s_fs_info; + msBlk = s->s_fs_info; sBlk = >sBlk; msBlk->devblksize = sb_min_blocksize(s, BLOCK_SIZE); @@ -914,8 +912,7 @@ static int squashfs_fill_super(struct su s->s_op = _ops; /* Init inode_table block pointer array */ - if (!(msBlk->block_cache = (squashfs_cache *) -
[PATCH] remove some usesless casts
Squashfs is extremely cast-happy. This patch makes it less so. Jörn -- If you're willing to restrict the flexibility of your approach, you can almost always do something better. -- John Carmack Signed-off-by: Jörn Engel [EMAIL PROTECTED] --- fs/squashfs/inode.c | 63 ++-- 1 files changed, 27 insertions(+), 36 deletions(-) --- linux-2.6.12-rc2cow/fs/squashfs/inode.c~squashfs_cu12005-04-20 07:52:46.0 +0200 +++ linux-2.6.12-rc2cow/fs/squashfs/inode.c 2005-04-20 08:11:10.254367656 +0200 @@ -111,7 +111,7 @@ struct inode_operations squashfs_dir_ino static struct buffer_head *get_block_length(struct super_block *s, int *cur_index, int *offset, int *c_byte) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; unsigned short temp; struct buffer_head *bh; @@ -176,7 +176,7 @@ unsigned int squashfs_read_data(struct s unsigned int index, unsigned int length, unsigned int *next_index) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; struct buffer_head *bh[((SQUASHFS_FILE_MAX_SIZE - 1) msBlk-devblksize_log2) + 2]; unsigned int offset = index ((1 msBlk-devblksize_log2) - 1); @@ -285,7 +285,7 @@ int squashfs_get_cached_block(struct sup int length, unsigned int *next_block, unsigned int *next_offset) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; int n, i, bytes, return_length = length; unsigned int next_index; @@ -390,7 +390,7 @@ static int get_fragment_location(struct unsigned int *fragment_start_block, unsigned int *fragment_size) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; unsigned int start_block = msBlk-fragment_index[SQUASHFS_FRAGMENT_INDEX(fragment)]; int offset = SQUASHFS_FRAGMENT_INDEX_OFFSET(fragment); @@ -434,7 +434,7 @@ static struct squashfs_fragment_cache *g int length) { int i, n; - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; for (;;) { down(msBlk-fragment_mutex); @@ -466,8 +466,7 @@ static struct squashfs_fragment_cache *g SQUASHFS_CACHED_FRAGMENTS; if (msBlk-fragment[i].data == NULL) - if (!(msBlk-fragment[i].data = - (unsigned char *) kmalloc + if (!(msBlk-fragment[i].data = kmalloc (SQUASHFS_FILE_MAX_SIZE, GFP_KERNEL))) { ERROR(Failed to allocate fragment @@ -509,7 +508,7 @@ static struct squashfs_fragment_cache *g static struct inode *squashfs_new_inode(struct super_block *s, squashfs_base_inode_header *inodeb, unsigned int ino) { - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; squashfs_super_block *sBlk = msBlk-sBlk; struct inode *i = new_inode(s); @@ -535,7 +534,7 @@ static struct inode *squashfs_new_inode( static struct inode *squashfs_iget(struct super_block *s, squashfs_inode inode) { struct inode *i; - squashfs_sb_info *msBlk = (squashfs_sb_info *)s-s_fs_info; + squashfs_sb_info *msBlk = s-s_fs_info; squashfs_super_block *sBlk = msBlk-sBlk; unsigned int block = SQUASHFS_INODE_BLK(inode) + sBlk-inode_table_start; @@ -837,13 +836,12 @@ static int squashfs_fill_super(struct su TRACE(Entered squashfs_read_superblock\n); - if (!(s-s_fs_info = (void *) kmalloc(sizeof(squashfs_sb_info), - GFP_KERNEL))) { + if (!(s-s_fs_info = kmalloc(sizeof(squashfs_sb_info), GFP_KERNEL))) { ERROR(Failed to allocate superblock\n); return -ENOMEM; } memset(s-s_fs_info, 0, sizeof(squashfs_sb_info)); - msBlk = (squashfs_sb_info *) s-s_fs_info; + msBlk = s-s_fs_info; sBlk = msBlk-sBlk; msBlk-devblksize = sb_min_blocksize(s, BLOCK_SIZE); @@ -914,8 +912,7 @@ static int squashfs_fill_super(struct su s-s_op = squashfs_ops; /* Init inode_table block pointer array */ - if (!(msBlk-block_cache = (squashfs_cache *) -
Re: [PATCH] remove some usesless casts
Jörn Engel wrote: Squashfs is extremely cast-happy. This patch makes it less so. Jörn Hi, Thanks for the patch. Unnecessary casts were one of the things mentioned when I submitted the patches to the LKML, and therefore I suspect most of them have been already fixed (but I will apply your patch to check). I will send revised patches to the LKML soon, most of the issues raised by the comments have been fixed, the current delay is being caused by the 4GB limit re-work. Regards Phillip - 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: [PATCH] remove some usesless casts
On Wed, 20 April 2005 16:20:10 +0100, Phillip Lougher wrote: Jörn Engel wrote: Squashfs is extremely cast-happy. This patch makes it less so. Thanks for the patch. Unnecessary casts were one of the things mentioned when I submitted the patches to the LKML, and therefore I suspect most of them have been already fixed (but I will apply your patch to check). Your definition of _unnecessary_ casts may differ from mine. Basically, every cast is unnecessary, except for maybe one or two - if that many. I will send revised patches to the LKML soon, most of the issues raised by the comments have been fixed, the current delay is being caused by the 4GB limit re-work. There are three more patches in my queue, which I'll send after first coffee or so. After those I'll ignore squashfs for a while (until you send current code or so). Jörn -- The story so far: In the beginning the Universe was created. This has made a lot of people very angry and been widely regarded as a bad move. -- Douglas Adams - 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: [PATCH] remove some usesless casts
Jörn Engel wrote: Your definition of _unnecessary_ casts may differ from mine. Basically, every cast is unnecessary, except for maybe one or two - if that many. Well we agree to differ then. In my experience casts are sometimes necessary, and are often less clumsy than the alternatives (such as unions). This is probably a generational thing, the fashion today is to make languages much more strongly typechecked than before. - 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: [PATCH] remove some usesless casts
On Wed, 20 April 2005 21:51:15 +0100, Phillip Lougher wrote: Jörn Engel wrote: Your definition of _unnecessary_ casts may differ from mine. Basically, every cast is unnecessary, except for maybe one or two - if that many. Well we agree to differ then. In my experience casts are sometimes necessary, and are often less clumsy than the alternatives (such as unions). This is probably a generational thing, the fashion today is to make languages much more strongly typechecked than before. I never claimed to replace the casts with unions. ;) Jörn -- Homo Sapiens is a goal, not a description. -- unknown - 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/