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/.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
