Looks good to me, ok nicm I would probably have used snprintf first myself, to be fair.
The tempstr stuff is hilarious though... On Tue, 14 Sept 2021 at 17:42, Ingo Schwarze <[email protected]> wrote: > Hi, > > Stefan Hagen wrote on Sat, Sep 11, 2021 at 09:00:18AM +0200: > > > Is this better? > > No, it is no good whatsoever. > > * It introduces yet another pointless static buffer. > * It pointlessly uses snprintf(3) where printf(3) > is obviously sufficient. > * It introduces error checking for operations > that obviously cannot fail when done properly. > * It needlessly does pointer arithmetic. > > So far, so bad, but the worst part is that it apparently wasn't > tested on a 64-bit platform (a platform where sizeof(long) == 64, > for example amd64). On such a platform, there is another bug on > the very same line. The function binfmtl() has platform-dependent > behaviour and does not behave as intended on a 64-bit platform and > prints 32 spurious zero-digit characters, breaking the desired > output alignment and making the content of the output > self-contradictory on top of that. > > Rather than try to change binfmtl(), which is mostly pointless > in the first place, let's just trivially inline the desired > functionality, it's just two lines of very simple C code. > > OK? > Ingo > > P.S. > Usually, we would want to upstream such a bugfix. But the overall > quality of this code is so abysmal that i'm not sure upstreaming > anything is worth the hassle. Doing minimal cleanup of the most > glaring style issues in this code would require changing 99% of it, > and given how it is written throughout, my suspicion is that it is > likely full of bugs, and having two bugs on this one line may not > be an exception. Just look at the totally ridiculous tempstr > gymnastics right below, which is incredibly stupid. Or look just > a few lines above and admire the hand-rolled code handling six-byte > UTF-8 characters (which are invalid according to the standard). > The code is just very, very bad throughout in large numbers of > respects. > > I'm ashamed to admit that i have occasionally been using this port > myself, and after looking at the code, i'm no longer convinced i > should do that. Not sure yet what to do about that. Cleaning this > up looks like a project of significant size, and quite a painful one. > > I just looked up the author of this code: > https://en.wikipedia.org/wiki/William_Poser > https://billposer.org/ > It looks like he is probably an eminent linguist, but he is clearly > not a software developer by any stretch of the imagination. > > > Index: Makefile > =================================================================== > RCS file: /cvs/ports/misc/uniutils/Makefile,v > retrieving revision 1.9 > diff -u -p -r1.9 Makefile > --- Makefile 28 Jun 2021 21:34:19 -0000 1.9 > +++ Makefile 14 Sep 2021 15:52:32 -0000 > @@ -3,7 +3,7 @@ > COMMENT= Unicode utilities > > DISTNAME= uniutils-2.27 > -REVISION= 3 > +REVISION= 4 > CATEGORIES= misc > > HOMEPAGE= http://billposer.org/Software/unidesc.html > Index: patches/patch-ExplicateUTF8_c > =================================================================== > RCS file: patches/patch-ExplicateUTF8_c > diff -N patches/patch-ExplicateUTF8_c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ patches/patch-ExplicateUTF8_c 14 Sep 2021 15:52:32 -0000 > @@ -0,0 +1,30 @@ > +$OpenBSD$ > + > +Remove %n format specifier > +and stop using the platform-dependent function binfmtl(). > +We want 32 bits here, not whatever sizeof(long) might be. > + > +Index: ExplicateUTF8.c > +--- ExplicateUTF8.c.orig > ++++ ExplicateUTF8.c > +@@ -87,7 +87,7 @@ main(int ac, char **av){ > + int UsefulBits; > + unsigned char c[6]; > + int i; > +- UTF32 ch; > ++ UTF32 ch, mask; > + unsigned char *cptr; > + unsigned char ShiftedByte; > + char tempstr[33]; > +@@ -214,7 +214,10 @@ main(int ac, char **av){ > + printf("%s ",tempstr); > + } > + printf("\n"); > +- printf("This is padded to 32 places with %d zeros: > %n%s\n",(32-GotBits),&spaces,binfmtl(ch)); > ++ spaces = printf("This is padded to 32 places with %d zeros: > ",(32-GotBits)); > ++ for (mask = 1UL << 31; mask; mask >>= 1) > ++ putchar(ch & mask ? '1' : '0'); > ++ putchar('\n'); > + sprintf(tempstr," "); > + sprintf(tempstr,"%08lX",ch); > + tempstr[28] = tempstr[7]; >
