Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support

[Maciej W. Rozycki]

On Thu, 12 Jul 2007, Andy Whitcroft wrote:

> > printk() should include KERN_ facility level
> > #750: FILE: drivers/serial/sb1250-duart.c:675:
> > +		printk(err);
> 
> Heh, yeah Ingo pointed this style out.  This is a wrapper where the
> facility will be supplied by the caller (I assume).  The thought there

 Actually "err" is "static const char *", except it is used twice in the 
function, so rather than cluttering the source with two identical strings 
and relying on GCC merging them I did it explicitly.

> was that only complain on printks which had a string literal as their
> first arguement.  That gets us very high accuracy and eliminates these
> falsies.

 That would be my suggestion too, if you asked me.  But as did not, I do 
not either.

> I think I tend to agree that the MAKEMASK ones are separate.  Good to
> see someone using their common sense in the face of whinging by the tool.

 Thanks for appreciation. ;-)

> WARNING: declaring multiple variables together should be avoided
> #372: FILE: drivers/serial/sb1250-duart.c:246:
> +	unsigned int mctrl, status;

 Well, this is probably superfluous -- why would anyone prefer:

	int r0;
	int r1;
	int r2;
	int r3;
	int r4;

to:

	int r0, r1, r2, r3, r4;

unconditionally?  I agree clustering variable declarations may obfuscate 
the code, but then again, a bit of common sense should be used.  It 
usually makes sense to group related variables together and declare other 
ones separately.

 And obviously if somebody writes unreadable code, then it is hard to 
change the habit with a script no matter how much you try.

  Maciej
-
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/