[PATCH 10/30] staging: nvec: Introduce new internal API for msg alloc/free

[Julian Andres Klode]

On Sat, Sep 24, 2011 at 01:06:57AM +0300, Dan Carpenter wrote:
> On Fri, Sep 23, 2011 at 11:53:58PM +0200, Julian Andres Klode wrote:
> > On Fri, Sep 23, 2011 at 10:54:05PM +0300, Dan Carpenter wrote:
> > > On Fri, Sep 23, 2011 at 06:38:02PM +0200, Julian Andres Klode wrote:
> > > > +static struct nvec_msg *nvec_msg_alloc(struct nvec_chip *nvec)
> > > > +{
> > > > +	size_t i;
> > > This should be "int i;" not "size_t i;"  It's a number between 0 and
> > > 64.  Also it would let you avoid the cast below.
> > Yes, somehow I'm a size_t person, especially when counting
> > array indices. On ARM it won't make a difference technically,
> > but on e.g. amd64, the size_t version is supposed to be
> > faster (according to some). But if we want to be pedantic,
> > we can make that "unsigned int i;"
> > 
> 
> Please just make it int.  Don't over think things.

I don't over think, I just know it's never < 0, so why
use a signed integer. A person annoyed me with their
"prevent signed integer if possible" attribute in
comments on LWN some time ago that I basically
accepted it somehow.

> 
> Anyway, I have a hard time believing that x86_64 can count to 64
> faster in unsigned longs than it can in ints.  I'd have to see some
> benchmarks to believe it.  :P  Best to let gcc optimize things
> anyway.

Well, not for a loop to 64. But if it can not determine that there
is no overflow, it seems that that's faster. I might have gotten
that from [1], although I'm not sure about it. And yes, optimizing
is always good.

> 
> > > > +static void nvec_msg_free(struct nvec_chip *nvec, struct nvec_msg *msg)
> > > > +{
> > > > +	dev_vdbg(nvec->dev, "INFO: Free %i\n", (int) (msg - nvec->msg_pool));
> > > 
> > > I don't have a cross compile environment set up so I can't compile
> > > this, but surely (msg - nvec->msg_pool) generates some kind of
> > > compile warning.  I'd think you'd have to cast the struct pointers to
> > > unsigned long or something.  I'm not sure also what the printk tells
> > > us.
> > They are two pointers, you can subtract two pointers and get an
> > ptrdiff_t. There is one problem with tx_scratch though, as that's
> > not part of the array, it is undefined behavior if called on nvec->tx
> > when nvec->tx == nvec->tx_scratch.
> > 
> 
> Yeah.  You're right.  I misread that.  Sorry.
We should probably still fix that undefined behavior, though, right?

[1] http://www.viva64.com/en/a/0050/
-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.