From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta1.formilux.org (mta1.formilux.org [51.159.59.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 22BCD13AD26 for ; Wed, 4 Feb 2026 10:40:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=51.159.59.229 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770201631; cv=none; b=cuMXJ3Bkkmr6gcO33d74YR6tuj9oK6FrbhhmgLGRZqwQ0+vb7ttvYXsBvxE0d0++4KUdPJMgpVX99sObYsoTUXraPacOVmZLXQEBfCQ61G9giFbg67R2UpEJ3RGTtiusekLsIPhPfzEgH0zNMgBWEldrT9RP9DszQ3nzMTj68aM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770201631; c=relaxed/simple; bh=mbIKHK/hcJw6q57vI5Jm+Q8EKfNqGIWK0mcPk7SAuAE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=U2D5UT9K9ZAwg49Lb7ry9yf8kym3JmlOhIbYK/jq94v4i1FV/EWTbZnJtXLLzFWu8Nno7B1cFP9BENPvgcgun5qPDl4oQLOJnC1d6wrnfIrGgY/174zFke5COi53zfr1q4iBTtq6BPTz65GmnCIY9Mwl+qQaUyxhHf6LlxkGQSc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=1wt.eu; spf=pass smtp.mailfrom=1wt.eu; dkim=pass (1024-bit key) header.d=1wt.eu header.i=@1wt.eu header.b=hhW2x5/+; arc=none smtp.client-ip=51.159.59.229 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=1wt.eu Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=1wt.eu Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=1wt.eu header.i=@1wt.eu header.b="hhW2x5/+" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1wt.eu; s=mail; t=1770201627; bh=KjXcpGbOGBbX5GxUbkwMOzWQMFjCI0DVMvXad4hZM5A=; h=From:Message-ID:From; b=hhW2x5/+rJXX6ZdzlKz9ItV1c2ccoKEp/rmueexedfx9qhOs/mHyYYZPdMHJ1rVyO 9WKiAEepfTFjTVgnz0c4ZP60kV6FsHU8koVEr+S+f+Q2BO2iqrTNNdopHFNrWu9SVh GklrlOPL164WzgZiMzFzecyOAxtL4+evjwpcIlL4= Received: from 1wt.eu (ded1.1wt.eu [163.172.96.212]) by mta1.formilux.org (Postfix) with ESMTP id DB6A2C0A4A; Wed, 04 Feb 2026 11:40:27 +0100 (CET) Date: Wed, 4 Feb 2026 11:40:27 +0100 From: Willy Tarreau To: David Laight Cc: Thomas =?iso-8859-1?Q?Wei=DFschuh?= , linux-kernel@vger.kernel.org, Cheng Li Subject: Re: [PATCH next 06/12] tools/nolibc/printf: Add support for left alignment and %[tzLq]d" Message-ID: References: <20260203103000.20206-1-david.laight.linux@gmail.com> <20260203103000.20206-7-david.laight.linux@gmail.com> <20260204101705.11d2d99b@pumpkin> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260204101705.11d2d99b@pumpkin> 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