From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E75AA25776 for ; Wed, 4 Feb 2026 15:39:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770219594; cv=none; b=mxdyucKeNsL3ucQ2in9i30G7XsZv2JrMeUONRDq+AY+hL4b/LZpNjICwGd85PcBDLIGqyoatAMzjWoO2XVtlbhLM1lZ752soz0Fmx+noIRikVEWsm6G1w1zHu0sdVo2ssvPDJOhjZpmhU1QsKo+xzX8VnC42XQRJXPA33X+oec4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770219594; c=relaxed/simple; bh=IsU4abg4CYysZ7nyGZtVBRgJ+aXMl9v5bZUAM0k0kTI=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=QKFcWk61XJTHA/h0QcYGqzQWvc3xdzX4YpE5jkotw1BDXK2fic4UdgIcsMjRzCCDKrYodXqV9AyQU8/w80dNr/jxXM3vtPTM6IN8YQTKBiL+9Spk+DiEeDu7T13opENFTBRaopuXIyTcq/HqS0Orz47GBglHqxfJJ1oDGNP+VLw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=cFBw72Hp; arc=none smtp.client-ip=209.85.128.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="cFBw72Hp" Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-47ff94b46afso11413735e9.1 for ; Wed, 04 Feb 2026 07:39:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1770219592; x=1770824392; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=fUyJGaWVeVEx9JhNlZPDEBC2wHiXRx9HtvO4XBb/ck8=; b=cFBw72HpPJG3yfNfGFmiWO9TeOPdQlivRb+t2wBjAyubb1HG0XKjcTYIQDa+F9x1gl Ginz0RyenU4WNn8fzUlSs3xi0lJvVxQ7buiD/ryP23QbuHXTCKixEGtQ+wEyZec0cOCA NyyAgXYnaE93CaShaYaAfjEjzpXXkny5J8pEwYTfKlBsDrPNG6SvjQHo/krtLXPnHltg Oz1U7u8275zLAaVDiw8/lnVPCCjQvm60PY4nOGX2Oa8wkJvDY9Ac7WQ0vu+/mj3Hm2dz FipSHmAOb5AxsR5eq5XUYolVt4X0R16BoW/NDEtQXodhw4zxGqNywNiHM3C5kkH5Hb+p JVRw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770219592; x=1770824392; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=fUyJGaWVeVEx9JhNlZPDEBC2wHiXRx9HtvO4XBb/ck8=; b=POPp7Pvzz1HXO0Glato9hRnryGTGil6PgZZ0Nvmb01ZPHr8+wszR2fNcn7B/WcHVhD mKQ4E+vVOF4VtuZp+baeVVDuT9Od8SHvqEtnp3vmscoiAS/EWu+aBuFwwEPebZIyLCWc 1Cb/PleE8DtDCETPrR91vuM/mAmxy9ljb5cATJnGxYCFRvY9zwxG3ThqV9upOB8H82tU jy8Nq6o7t7Guzdp/dMcKFrdkON/hshAh9cHQXgxCfYLoCOxYrM3ItuzWLSfpPu6EFiH3 0YoU1AbM+ajSVFpHoXsYACkDzcPvX59pfsiSPyChYTBjh43k8PnWyHhVG6UwOgp+iYdV APqg== X-Forwarded-Encrypted: i=1; AJvYcCU2SgEO/vl+wXBRqzqjoVPbRi7+go+bbYdFPKlYx9uSR3JVlffRsS17JyIAl9HQWlKyloGuuBeECMdjOT4=@vger.kernel.org X-Gm-Message-State: AOJu0YxPoU2yj5NRcZ29dYSvoXe7Pox5Nmwh7am4jznwps6tpNEY4533 PMO74eZNuf5wRapKbHDh0IJmjT3ST3ytNBIiU69uAg7z7Ya9YZR2uxGN X-Gm-Gg: AZuq6aIHsodp2vF2q++5FoGV6CpgpN0XM5LbOwECBecsD6M3zkpXjo5WSCOqs18Uc2U g0MWQcRZZl9bk7kak7/8WgpVny1aByhaDav+s7MIEbTpgiH14onWiDbdRF2cFrY4jp/8JeDfMnR kVjaQWqEpk2pXbFpbpP8O0uhKM1Js5R4BepoZ3ZuHTyPDVS7c4Id2S6oAo3jeR8WlkHFal+a7fu yhRJVpHnX8BAM5wXR2yDsrtGe9rw4ve0gIR9B8+BZ7fyRFsbFZJUJXr7HxY85It+aauseEXEyGY phEP1TmlbRxbGsV7OaaqDXpCXvbUmembKkD1Xd3qegmAvx8i552D+9Y2bdcGbcp4/tLkuz9KIeA 8HkqQWXvqiJGXVjuZqiyteSY6GULhM+KNaVNaBl9ClWVJOVRO13GVfMQwQ2pLMgzUhfBLxvJG4J o2iApD3xb3WgZgP22EWB373eFBk+za2TanWBl/c1NaECbHcXbc2dF+1MFv82Gkafk= X-Received: by 2002:a05:600c:3484:b0:477:a53c:8ca1 with SMTP id 5b1f17b1804b1-483051718d4mr96987345e9.14.1770219591856; Wed, 04 Feb 2026 07:39:51 -0800 (PST) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4830fe62fe7sm32428165e9.4.2026.02.04.07.39.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Feb 2026 07:39:51 -0800 (PST) Date: Wed, 4 Feb 2026 15:39:50 +0000 From: David Laight To: Willy Tarreau Cc: Thomas =?UTF-8?B?V2Vpw59zY2h1aA==?= , 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: <20260204153950.76e95da5@pumpkin> In-Reply-To: References: <20260203103000.20206-1-david.laight.linux@gmail.com> <20260203103000.20206-7-david.laight.linux@gmail.com> <20260204101705.11d2d99b@pumpkin> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) 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-Transfer-Encoding: 7bit On Wed, 4 Feb 2026 11:40:27 +0100 Willy Tarreau wrote: > 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. I'd also #undef'ed the wrong name. > > > > > + /* 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've done some changes - I'll need to back-merge them into the patches. That code now looks like: /* Conversion flag characters are all non-alphabetic. * Create a bit-map of the valid ones for later. */ for (; ch >= 0x20 && ch <= 0x3f; ch = *fmt++) { if (!_NOLIBC_PF_CHAR_IS_ONE_OF(ch, ' ', '#', '+', '-', '0')) break; flags |= _NOLIBC_PF_FLAG(ch); } Which is definitely more readable (until you look inside the #define). I could even hide the range check inside the macro by using the high bits of the first character. That would then read: while (_NOLIBC_PF_CHAR_IS_ONE_OF(ch, ' ', '#', '+', '-', '0')) { flags |= _NOLIBC_PF_FLAG(ch); ch = *fmt++; } or maybe: for (fl = 1; fl; ch = &fmt++) { fl = _NOLIBC_PF_CHAR_IS_ONE_OF(ch, ' ', '#', '+', '-', '0'); flags |= fl; } (especially if gcc generates a lot less code for it - I expect it will pessimise it back to the other version.) > > > 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 only counted 7 :-) I used the __VA_ARGS__, 0, 0 trick to support up to 8. > 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 :-/ It is a simple one that does make a difference. It would have to be the first patch, which will make 'rebasing' the patches a nightmare. > > > 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. I've been running the bloat-o-meter on pretty much every build. The current build of __nolibc_printf (without my changes to stdlib.h) is at +90 bytes. But I think it has lost an inlined copy of u64toa_r (about 144 bytes). Actual size is about 1k. The bit-flags variables definitely help over multiple comparisons. The width/precision loop helps due to the size of va_arg() (that is one of the bits that actually increases the size). The code to buffer printf() (to file) actually adds more than the other changes. But I like it because of the difference it makes to strace. Don't even look at the mess gcc creates for the obvious: precision = width - (size != 0) - (size > 255); And I can't get it to use 'bt' (bit test) or 'bts' (bit test and set). Code like: if (_NOLIBC_PF_CHAR_IS_ONE_OF(ch, ' ', '#', '+', '-', '0')) flags |= _NOLIBC_PF_FLAG(ch); Could be: mask = 0x....; if (bt(ch, mask)) bts(ch, flags); But it insists on doing: if ((mask >> ch) & 1) flags |= 1 << ch; I can't even get it to do: if (mask & (1 << ch)) flags |= 1 << ch; (I need the condition for loop control.) I've not tried variants of: bit = mask & (1 << ch); flags |= bit; if (bit)... There is on 'bts' - in the code that adds the '#' flag into the conversion character. Doing that improves the code far more than you might expect. To the point where adding |= 1 << 'b' might actually reduce the code size even though 'b' is never tested for. ... > > 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. I meant 'really horrible' ... A few of the lines are in the 90s, one might be 103. I still like 80 characters, but anal continuation lines are silly. (I found some (ex)dayjob code written 20+ years ago with 200+ character lines, no idea how they edited 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). I have been looking at size. I ripped some changes out because they didn't make things smaller and made them very much less readable. It when down quite a bit when I did the restructure and then crept back up as I added extra features. Overall there isn't that much difference. One thing that would reduce overall size is making the non-trivial functions in stdlib.h (and maybe elsewhere) noinline. (I set that for some of my builds to stop the numeric convertion functions being inlined - no idea why they aren't considered too big.) For gcc adding noclone may also help (not supported by clang). noclone can have a strange effect. I set it on my u64toa_base() functions but only enabled it for decimal. There were two call sites, both passed in the two constants. But the called function ignored the passed values and reloaded both constants. Not at all 'what the crowd intended' :-) David > > Thanks, > Willy