Date:	Mon, 29 Dec 2008 15:35:31 +0100
From:	"Hannes Eder" <hannes@...neseder.net>
To:	"Linus Torvalds" <torvalds@...ux-foundation.org>
Cc:	"Krzysztof Halasa" <khc@...waw.pl>,
	"Harvey Harrison" <harvey.harrison@...il.com>,
	"Håkon Løvdal" <hlovdal@...il.com>,
	netdev@...r.kernel.org, kernel-janitors@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/27] drivers/net: fix sparse warnings: make do-while a compound statement

On Wed, Dec 24, 2008 at 12:38 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
>
> On Wed, 24 Dec 2008, Krzysztof Halasa wrote:
>>
>> So is a case like
>>       do
>>               x;
>>       while (y);
>> It can't be made more clear with brackets.
>
> Oh yes it can. People look at that, and it's so uncommon that they
> literally believe it is a mis-indent.
>
> [snip]
>
>> kmalloc(sizeof i) just doesn't look good, the operator looks like
>> a variable name.
>
> And I agree with you. "sizeof i" doesn't look good. It's uncommon, and
> doesn't match peoples expectations.
>
>> But there is this return statement. Some people tend to write
>> return (x); I simply write return x;
>
> I do to. And it's the common thing to do, and only totally confused people
> think that 'return' is a somehow remotely like a "function" of its
> arguments (the way 'sizeof' is - sizeof _is_ a function of its arguments,
> albeit a very rare one).
>
>> It's clear, and so is a simple do-while.
>
> No. "return x;" is clear because it's the common thing, which means that
> peopel are good at reading it.

I got curious and conducted a little experiment, here is the outcome
for the linux-2.6 kernel tree:

hannes@...ox:~/linux-2.6$ find . -name "*.[ch]" -print0 | xargs -0 cat
| ../sparse/cstats -

stats for '-':
  do's:         8092, non compound:              79 (  1.0%)
  sizeof's:    51216, without parenthesis:     1543 (  3.0%)
  return's:   286167, with parenthesis:       13552 (  4.7%)

'cstats' is a little program I wrote using the sparse library, see below.

The value for "return's with parenthesis" is a bit of an estimation,
as 'cstats' operates only at the token level, so
"return (x) ? y : z;" counts as "return with parenthesis".

some sanity checks, to ensure the magnitudes are right:

hannes@...ox:~/linux-2.6$ git grep -w do -- *.[ch] | wc -l
20599

hannes@...ox:~/linux-2.6$ git grep '\bdo[ \t]*{' -- *.[ch] | wc -l
7805

I assume 'do' is used frequently in comments:

hannes@...ox:~/linux-2.6$ git grep '\*.*\bdo\b' -- *.[ch] | wc -l
11358

hannes@...ox:~/linux-2.6$ git grep '^[ \t]*do$' -- *.[ch] | wc -l
34

hannes@...ox:~/linux-2.6$ git grep -w sizeof -- *.[ch] | wc -l
49631

constructs like sizeof(array)/sizeof(array[0]) or common:

hannes@...ox:~/linux-2.6$ git grep  'sizeof.*sizeof' -- *.[ch] | wc -l
1827

hannes@...ox:~/linux-2.6$ git grep '\bsizeof [^(]' -- *.[ch] | wc -l
1534

hannes@...ox:~/linux-2.6$ git grep -w return -- *.[ch] | wc -l
295304

hannes@...ox:~/linux-2.6$ git grep '\breturn (' -- *.[ch] | wc -l
11067

Best,
Hannes


#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>

#include "token.h"
#include "parse.h"

int main(int argc, char **argv)
{
        struct string_list *filelist = NULL;
        char *filename;

        sparse_initialize(argc, argv, &filelist);
        FOR_EACH_PTR_NOTAG(filelist, filename) {
                int fd;
                struct token *token;
                int do_stats = 0;
                int do_stats_nc = 0;
                int sizeof_exprs = 0;
                int sizeof_exprs_np = 0;
                int return_stats = 0;
                int return_stats_wp = 0;

                if (strcmp(filename, "-") == 0) {
                        fd = 0;
                } else {
                        fd = open(filename, O_RDONLY);
                        if (fd < 0)
                                die("No such file: %s", filename);
                }

                // Tokenize the input stream
                token = tokenize(filename, fd, NULL, includepath);
                close(fd);

                for ( ; !eof_token(token); token = token->next) {
                        if (token_type(token) == TOKEN_IDENT) {
                                if (token->ident == &do_ident) {
                                        do_stats++;
                                        if (!match_op(token->next, '{'))
                                                do_stats_nc++;
                                }
                                else if (token->ident == &sizeof_ident) {
                                        sizeof_exprs++;
                                        if (!match_op(token->next, '('))
                                                sizeof_exprs_np++;
                                }
                                else if (token->ident == &return_ident) {
                                        return_stats++;
                                        if (match_op(token->next, '('))
                                                return_stats_wp++;
                                }
                        }
                }

                printf("stats for '%s':\n", filename);
                printf("  do's:     %8d, non compound:        %8d (%5.1f%%)\n",
                       do_stats, do_stats_nc,
                       100.0 * do_stats_nc / (do_stats?:1) );
                printf("  sizeof's: %8d, without parenthesis: %8d (%5.1f%%)\n",
                       sizeof_exprs, sizeof_exprs_np,
                       100.0 * sizeof_exprs_np / (sizeof_exprs?:1) );
                printf("  return's: %8d, with parenthesis:    %8d (%5.1f%%)\n",
                       return_stats, return_stats_wp,
                       100.0 * return_stats_wp / (return_stats?:1) );

        } END_FOR_EACH_PTR_NOTAG(filename);
        return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists