* Bill Huey <[EMAIL PROTECTED]> wrote: > > - Documentation/CodingStyle compliance - the code is not ugly per se > > but still looks a bit 'alien' - please try to make it look Linuxish, > > if i apply this we'll probably stick with it forever. This is the > > major reason i havent applied it yet. > > I reformatted most of the patch to be 80 column limited. I simplified > a number of names, but I'm open to suggestions and patches to how to > go about this. Much of this code was a style experiment, but now I > have to make this more mergable.
thanks. It's looking better, but there's still quite a bit of work left: there's considerable amount of whitespace noise in it - lots of lines with space/tab at the end, lines with 8 spaces instead of tabs, etc. comment style issues: +/* To be use for avoiding the dynamic attachment of spinlocks at runtime + * by attaching it inline with the lock initialization function */ the proper multi-line style is: /* * To be used for avoiding the dynamic attachment of spinlocks at * runtime by attaching it inline with the lock initialization function: */ (note i also fixed a typo in the one above) more unused code: +/* +static DEFINE_LS_ENTRY(__pte_alloc); +static DEFINE_LS_ENTRY(get_empty_filp); +static DEFINE_LS_ENTRY(init_waitqueue_head); ... +*/ these: +static int lock_stat_inited = 0; should not be initialized to 0, that is implicit for static variables. weird alignment here: +void lock_stat_init(struct lock_stat *oref) +{ + oref->function[0] = 0; + oref->file = NULL; + oref->line = 0; + + oref->ntracked = 0; funky branching: + spin_lock_irqsave(&free_store_lock, flags); + if (!list_empty(&lock_stat_free_store)) { + struct list_head *e = lock_stat_free_store.next; + struct lock_stat *s; + + s = container_of(e, struct lock_stat, list_head); + list_del(e); + + spin_unlock_irqrestore(&free_store_lock, flags); + + return s; + } + spin_unlock_irqrestore(&free_store_lock, flags); + + return NULL; that should be s = NULL in the function scope and a plain unlock and return s. assignments mixed with arithmetics: +static +int lock_stat_compare_objs(struct lock_stat *x, struct lock_stat *y) +{ + int a = 0, b = 0, c = 0; + + (a = ksym_strcmp(x->function, y->function)) || + (b = ksym_strcmp(x->file, y->file)) || + (c = (x->line - y->line)); + + return a | b | c; the usual (and more readable) style is to separate them out explicitly: a = ksym_strcmp(x->function, y->function); if (!a) return 0; b = ksym_strcmp(x->file, y->file); if (!b) return 0; return x->line == y->line; (detail: this btw also fixes a bug in the function above AFAICS, in the a && !b case.) also, i'm not fully convinced we want that x->function as a string. That makes comparisons alot slower. Why not make it a void *, and resolve to the name via kallsyms only when printing it in /proc, like lockdep does it? no need to put dates into comments: + * Fri Oct 27 00:26:08 PDT 2006 then: + while (node) + { proper style is: + while (node) { this function definition: +static +void lock_stat_insert_object(struct lock_stat *o) can be single-line. We make it multi-line only when needed. these are only samples of the types of style problems still present in the code. Ingo - 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/