[PATCH 4/7] staging: nvec: add NVEC_CALL helper macro

[Greg KH]

On Sat, Jun 16, 2012 at 12:00:03AM +0200, Julian Andres Klode wrote:
> On Fri, Jun 15, 2012 at 02:45:33PM -0700, Greg KH wrote:
> > On Fri, Jun 15, 2012 at 08:40:26AM +0200, Julian Andres Klode wrote:
> > > On Thu, Jun 14, 2012 at 03:09:38PM -0700, Greg KH wrote:
> > > > On Thu, Jun 14, 2012 at 11:57:38PM +0200, Marc Dietrich wrote:
> > > > > Add a helper macro to wrap nvec_{a}sync_writes and to get rid of
> > > > > the various strings distributed all over the nvec code.
> > > > 
> > > > Why can't these be inline functions instead?  That will catch errors
> > > > easier, and make it a bit more "obvious" as to what is going on (hint, I
> > > > have no idea in reading these what they are doing...)
> > > 
> > > They are not really obvious, but they kind of catch more errors and are shorter to
> > > writer.
> > 
> > Shorter to write where?
> > 
> > And obvious is good, we want obvious in the kernel.  Non-obvious is bad,
> > bugs live there...
> > 
> > > A nvec "call" consists of a type, a subtype and a payload, so
> > > the calls expand like this:
> > > 
> > >     nvec_write_async(nvec, {NVEC_FOO, NVEC_FOO_BAR, ...}, length of ... + 2)
> > >     NVEC_CALL(nvec, FOO, BAR, ...)
> > > 
> > > With an inline function, you would have to use arrays (as nvec_write_async()
> > > does) or variable arguments lists which are not as optimizable, and you would
> > > need to repeat NVEC all over the place. For example, with an inline function
> > > instead of a macro:
> > > 
> > >     nvec_call(nvec, NVEC_FOO, NVEC_FOO_BAR, [size here?], ...)
> > > 
> > > This manual size tracking also makes it less reliable.
> > 
> > I'm sorry, but I still don't understand.  Why would you ever want any
> > NVEC_CALL macros either?
> > 
> > What exactly is the driver doing here that is so "odd" from other data
> > streams that need to be written to devices that it has to go through
> > wierd gyrations like this?
> 
> The core point was probably that we currently have various sequences
> like 
> 	char blah[] = {some hex value, other hex value, ...}
> 	nvec_write_async(nvec, blah, sizeof(blah))
> which is a bit long to write or distracting under some circumstances (and
> not very obvious, as we did not use named constants for those hex values
> most of the time). With a macro you could just write it:
> 
> 	NVEC_CALL(nvec, FOO, BAR)
> 
> And save a line. Would you prefer us to use:
> 
> 	{
> 		char msg[] = {NVEC_FOO, NVEC_FOO_BAR};
> 		nvec_write_async(nvec, msg, sizeof(msg));
> 	}
> 
> instead?

Yes, don't you agree that this is more readable?

> And yes, macro-less code is much more readable than the
> macro code for non-insiders, which is probably helpful.

Ah, you do agree, nice :)

Remember, write kernel code so that others can fix it 10 years in the
future.  Don't be cute, play tricks, or do fancy things if at all
possible.  Kernel code is written to be debugged by someone you have
never met, whose native language is not your own, who does not have your
device, and who you don't want having to email you asking questions.

thanks,

greg k-h