Message-ID: <aYMiGzNW8Pn_xXvx@1wt.eu>
Date: Wed, 4 Feb 2026 11:40:27 +0100
From: Willy Tarreau <w@....eu>
To: David Laight <david.laight.linux@...il.com>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
        linux-kernel@...r.kernel.org, Cheng Li <lechain@...il.com>
Subject: Re: [PATCH next 06/12] tools/nolibc/printf: Add support for left
 alignment and %[tzLq]d"

On Wed, Feb 04, 2026 at 10:17:05AM +0000, David Laight wrote:
> > This flag will be exposed to user code, you'll have to previx it with
> > _NOLIBC_.
> 
> The lines are long enough already, something shorter would be ideal.
> The #undef at the bottom stops it being exposed.

No, it's not just about not being exposed, it's about *conflicting*.
We just do not reserve macro names not starting with anything but
_NOLIBC. If I already use __PF_BASE in my application, it will cause
trouble here.

> > > +			/* Flag characters */
> > > +			for (; c >= 0x20 && c <= 0x3f; c = *fmt++) {
> > > +				if ((__PF_FLAG(c) & (__PF_FLAG('-'))) == 0)
> > > +					break;
> > > +				flags |= __PF_FLAG(c);
> > > +			}  
> > 
> > Honestly I don't find that it improves readability here and makes one
> > keep doubts in background as "what if c == 'm' which will match '-'?".
> > I think that one would better be written as the usual:
> > 
> > 			for (; c >= 0x20 && c <= 0x3f; c = *fmt++) {
> > 				if (c == '-')
> > 					break;
> > 				flags |= __PF_FLAG(c);
> > 			}
> > 
> > Or even simpler since there's already a condition in the for() loop:
> > 
> > 			for (; c >= 0x20 && c != '-' && c <= 0x3f; c = *fmt++)
> > 				flags |= __PF_FLAG(c);
> 
> It is all written that way for when more flags get added - look at the later
> patches which check for any of "#-+ 0".
> At that point the line get long and unreadable - as below :-)

I know, I've seen them and am already bothered by this. The
purpose of that lib has always been to focus on:
  1) size
  2) maintainability
  3) portability

Performance has never been a concern. I totally agree that testing
bitfields is often much shorter than multiple "if", though here I'm
seeing the code significantly inflate with loops etc (which might
remain small), but maintainability is progressively reducing. This
code receives few contributions from many participants, and it's
important that it's easy to understand what's being done in order
to easily add your missing feature. I'm feeling that we're starting
to steer away from this principle here, which is why I'm raising an
alarm.

> I didn't want to add the flags here before supporting them later.
> But they could all be accepted and ignored until implemented.
> That might be better anyway.

I've seen that in a later patch you have up to 10 values tested in
chain. I just think that it could be sufficient to have a macro
taking 10 char args, that remains easy enough to use and understand
where it is, e.g. you pass the base then all values:

    _NOLIBC_ANY_OF(0x20, 'c', 'd', 'r', 'z', -1, -1, -1, -1 ...)

> Actually it might be worth s/c/ch/ to make the brain see the difference
> between c and 'c' more easily.

I'm not sure it's the only detail which is complexifying my reading :-/

> Perhaps I'm expand the comment a bit.

Yes, comments are cheap and welcoming to new readers.

> It is all a hint as to what is happening later on with the character tests.

I roughly get what you're trying to do and am not contesting the goals,
I'm however questioning the size efficiency of the resulting code (not
fully certain it remains as small as reasonably possible), and the
ease of maintenance.

> > 
> > > +				if (__PF_FLAG(c) & (__PF_FLAG('l') | __PF_FLAG('t') | __PF_FLAG('z') |
> > > +						    __PF_FLAG('j') | __PF_FLAG('q'))) {  
> > 
> > Even though I understand the value in checking bit positions (I use that
> > all the time as well), above this is just unreadable. Maybe you need a
> > different macro, maybe define another macro _NOLIBC_PF_LEN_MOD made of
> > the addition of all the flags to test against, I don't know, but the
> > construct, the line break in the middle of the expression and the
> > parenthesis needed for the macro just requires a lot of effort to
> > understand what's being tested. Alternately another possibility would
> > be to have another macro taking 4-5 char args and composing the flags
> > in one call, passing 0 or -1 for unused ones. This would also make
> > several parenthesis disappear which would help.
> 
> Hmmm, some macro magic might work, loosely:
> #define FLNZ(q) (q ? 1 << (q & 31) ? 0)
> #define FLM3(q1, q2, q3, ...) ((FLNZ(q1) | FLNZ(q2) | FLNZ(q3))
> #define FLT(fl, ...) (fl & FLM3(__VA_ARGS__, 0, 0, 0))
> #define CT(c, ...) FLT(1 << (c & 31), __VA_ARGS__) 
> Then the above would be:
> 	if (CT(c, 'l', 't', 'z', 'j', 'q')) {
> Clearly needs some better and longer names and a big comment block.

Yes maybe something like this (with _NOLIBC_ please).

> > > +#undef _PF_FLAG  
> > 
> > This one can be dropped once named as _NOLIBC_xxx
> 
> I'll see if I can get a max of 2 expansions on a line.
> Otherwise the lines get horribly long.

OK but with todays screens it's less of a problem, and often early
wrapping affects legibility more than long lines :-/  We don't have
a strict 80-char limit in this project, so if you need 100 to make
something more readable once in a while, please just do it.

But please also keep in mind my comments about the goals of size,
maintainability and portability (e.g. don't forget to compare
size before/after, and at least to mention when some significant
changes have impacts in one direction or the other because that
matters).

Thanks,
Willy

Powered by blists - more mailing lists