Re: [PATCH 1/3] printf: support field padding

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jan 02, 2014 at 06:09:48PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:31:44PM -0800, Christoffer Dall wrote:
> > > +
> > > +    if (npad < 0) {
> > > +	char pad = props.pad;
> > > +	if (pad == '0') /* ignore '0' flag with '-' flag */
> > > +	    pad = ' ';
> > 
> > there are only the two options, so you can drop the check if
> > you like.
> 
> true. removed.
> 
> > > +static int fmtnum(const char **fmt)
> > > +{
> > > +    const char *f = *fmt;
> > > +    int len = 0, num;
> > > +
> > > +    if (*f == '-')
> > > +	++f, ++len;
> > 
> > oh wow, this deserves a small comment saying that negative values are
> > used to add trailing padding instead of leading.
> 
> You mean something beyond "man 3 printf; /flag"? :-)
> 

yes, that's a functional description, not helping the reader of the
implememtation.  But ok, once this works, it's not likely to pass many
eyes again.

> > 
> > > +
> > > +    while (*f >= '0' && *f <= '9')
> > > +	++f, ++len;
> > > +
> > > +    num = atol(*fmt);
> > > +    *fmt += len;
> > > +    return num;
> > >  }
> > 
> > some funny indentation is back here...  Better check your entire patch
> > for that.
> 
> The whole file has the funny indentation, I just followed suit. The
> alternative is to add a patch that "fixes" all the pre-existing lib/*
> files first, but for this patch I didn't think it was worth it.
> 
fair enough, but it really hurts when reading patches so we should fix
this some time...

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux