Re: [PATCH] bitmap parsing/printing routines, version 4

[Joe Korty]

On Mon, Jan 19, 2004 at 01:17:26PM -0800, Matthew Dobson wrote:
> Joe,
> 	I've attatched a small patch with some *small* changes, and the 
> addition of a whole lotta comments.  I'd like to see what you think.
> 
> Changes:
> 1) Added a missing '"' in the comment for the bitmap_parse function
> 2) Renamed 'oc' to 'old_c' for readability
> 3) Remove "totaldigits == 0" check at the end of bitmap_parse.  I 
> believe this check is redundant.  The only way that totaldigits could be 
> 0 at the end of the function is if ndigits is also 0 (because they're 
> both incremented at the same time), and this condition is already 
> checked for at the end of each chunk parsed.  Is this correct?
> 
> Additions:
> 4) A whole bunch of comments.  Are these all correct?
> 
> None of the things in my patch (with the possible exception of #3) 
> change the functionality of the code, which looks great.
> 
> Andrew, I agree with Paul's "thumbs-up" of Joe's patch.  My patch is 
> solely meant to increase the readability of the bitmap_parse function.
> 
> Cheers!
> 
> -Matt

Indeed you are correct, the final (totaldigits == 1) test can be removed.
Good catch.

However, IMHO you added too many comments.  Unlike Andrew, I do believe
one can have too many comments.  Comments become 'too many' when they
dilute to the point that the code can no longer be clearly read.

If you reduce the comments to just those that say something not easily
deduced from the code, then they would be acceptable to me, and would
make a useful addition IMO.  That would be all but three, or perhaps four,
of them.

Andrew, if you do like the fully commented version, then please remove
my name from the comment in the patch.  The dilute style of coding is
not one I wish to have my name associated with.

Thanks,
Joe
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/