[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20071204150734.GE6113@hacking>
Date: Tue, 4 Dec 2007 23:07:34 +0800
From: WANG Cong <xiyou.wangcong@...il.com>
To: Tejun Heo <htejun@...il.com>
Cc: sam@...nborg.org, Linux Kernel <linux-kernel@...r.kernel.org>,
notting@...hat.com, rusty@...tcorp.com.au, kay.sievers@...y.org,
greg@...ah.com
Subject: Re: [PATCH] kbuild: implement modules.order
{snip}
Comments on your C code below.
>--- /dev/null
>+++ b/scripts/remove-dup.c
>@@ -0,0 +1,98 @@
>+/*
>+ * remove-dup - Drop duplicate lines from unsorted input files
>+ *
>+ * Dec 2007 Tejun Heo <teheo@...e.de>
>+ *
>+ * This software is released under GPLv2.
>+ */
>+
>+#include <stdio.h>
>+#include <string.h>
>+#include <stdlib.h>
>+
>+struct hash_ent {
>+ struct hash_ent *next;
>+ char str[];
>+};
>+
>+#define fatal(fmt, args...) do { \
>+ fprintf(stderr, fmt , ##args); \
>+ exit(1); \
>+ } while (0)
>+
>+static inline unsigned int sdb_hash(const char *str)
>+{
>+ unsigned int hash = 0;
>+ int c;
>+
>+ while ((c = *str++))
Maybe ` while ((c = *str++) != '\0') ` is better. ;)
>+ hash = c + (hash << 6) + (hash << 16) - hash;
>+
>+ return hash;
>+}
>+
>+int main(int argc, char **argv)
>+{
>+ unsigned int nr_entries = 0;
>+ struct hash_ent **hash_tbl;
>+ char line[10240];
Needs to #define the magic number?
>+ int i;
>+
>+ /* first pass, count lines */
>+ for (i = 1; i < argc; i++) {
>+ FILE *fp = fopen(argv[i], "r");
>+
>+ if (!fp)
>+ fatal("failed to open %s for reading\n", argv[i]);
>+
>+ while (fgets(line, sizeof(line), fp))
>+ nr_entries++;
>+
>+ fclose(fp);
>+ }
>+
>+ nr_entries = nr_entries * 10 / 8;
>+ hash_tbl = calloc(nr_entries, sizeof(struct hash_ent *));
>+ if (!hash_tbl)
>+ fatal("failed to allocate hash table for %u entries\n",
>+ nr_entries);
>+
>+ /* second pass, hash and print unique lines */
>+ for (i = 1; i < argc; i++) {
>+ FILE *fp = fopen(argv[i], "r");
>+
>+ if (!fp)
>+ fatal("failed to open %s for reading\n", argv[i]);
>+
>+ while (fgets(line, sizeof(line), fp)) {
>+ int len = strlen(line);
strlen returns 'size_t', which is unsigned.
>+ struct hash_ent **ppos, *new_ent;
>+
>+ if (line[len - 1] == '\n')
>+ line[--len] = '\0';
>+
>+ ppos = hash_tbl + (sdb_hash(line) % nr_entries);
>+ while (*ppos) {
>+ if (strcmp((*ppos)->str, line) == 0)
>+ break;
>+ ppos = &(*ppos)->next;
>+ }
>+ if (*ppos)
>+ continue;
>+
>+ new_ent = malloc(sizeof(struct hash_ent) + len + 1);
>+ if (!new_ent)
>+ fatal("failed to allocate hash entry\n");
>+ new_ent->next = NULL;
>+ memcpy(new_ent->str, line, len + 1);
>+
>+ *ppos = new_ent;
>+
>+ printf("%s\n", line);
>+ }
>+
>+ fclose(fp);
>+ }
>+
I think, you forgot to free(3) the memory you calloc(3)'ed and
malloc(3)'ed above.
>+ return 0;
>+}
Thanks!
Cong
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists