Thread: Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail

From
Nicolai Tufar
Date:
Resubmission of yesterday's patch so that it would
cont conflict with Bruce's cvs commit. Pleas apply.

Best regards,
Nicolai.


On Sat, 12 Mar 2005 01:58:15 +0200, Nicolai Tufar <ntufar@gmail.com> wrote:
> On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian
> <pgman@candle.pha.pa.us> wrote:
> > > The CVS-tip implementation is fundamentally broken and won't work even
> > > for our internal uses.  I've not wasted time complaining about it
> > > because I thought we were going to replace it.  If we can't find a
> > > usable replacement then we're going to have to put a lot of effort
> > > into fixing what's there.  On the whole I think the effort would be
> > > better spent importing someone else's solution.
> >
> > Oh, so our existing implementation doesn't even meet our needs. OK.
>
> Which made me wander why did I not aggree with
> Tom Lane's suggestion to make do three passes
> instead of two. Tom was right, as usual. It happened to
> be much easier than I expected. The patch is attached.
> Please apply.
>
> Tom, what do you think? Will it be fine with you?
>
> Best regards,
> Nicolai
>
>
>

Attachment

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests

From
Bruce Momjian
Date:
I have applied a modified version of your patch, attached.

initdb will not work without %*s support, so I had to add that.  Please
look over my work.  I don't think i handle %*1$ but I am not even sure
what that means or if our translators would ever use such a thing.  You
can probably test that better than I can.

Your patch didn't handle signed vs. unsigned va_arg values properly..

There were also maxwidth tests around 's' that I had to remove to
support %*.  I think those tests are down in fmtstr too but please
check.

initdb and regression pass.

---------------------------------------------------------------------------

Nicolai Tufar wrote:
> Resubmission of yesterday's patch so that it would
> cont conflict with Bruce's cvs commit. Pleas apply.
>
> Best regards,
> Nicolai.
>
>
> On Sat, 12 Mar 2005 01:58:15 +0200, Nicolai Tufar <ntufar@gmail.com> wrote:
> > On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian
> > <pgman@candle.pha.pa.us> wrote:
> > > > The CVS-tip implementation is fundamentally broken and won't work even
> > > > for our internal uses.  I've not wasted time complaining about it
> > > > because I thought we were going to replace it.  If we can't find a
> > > > usable replacement then we're going to have to put a lot of effort
> > > > into fixing what's there.  On the whole I think the effort would be
> > > > better spent importing someone else's solution.
> > >
> > > Oh, so our existing implementation doesn't even meet our needs. OK.
> >
> > Which made me wander why did I not aggree with
> > Tom Lane's suggestion to make do three passes
> > instead of two. Tom was right, as usual. It happened to
> > be much easier than I expected. The patch is attached.
> > Please apply.
> >
> > Tom, what do you think? Will it be fine with you?
> >
> > Best regards,
> > Nicolai
> >
> >
> >

[ Attachment, skipping... ]

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/port/snprintf.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/snprintf.c,v
retrieving revision 1.19
diff -c -c -r1.19 snprintf.c
*** src/port/snprintf.c    12 Mar 2005 04:00:56 -0000    1.19
--- src/port/snprintf.c    16 Mar 2005 05:46:33 -0000
***************
*** 151,170 ****

  #define    FMTSTR        1
  #define    FMTNUM        2
! #define    FMTFLOAT    3
! #define    FMTCHAR        4

  static void
  dopr(char *buffer, const char *format, va_list args, char *end)
  {
      int            ch;
-     int64        value;
-     double        fvalue;
      int            longlongflag = 0;
      int            longflag = 0;
      int            pointflag = 0;
      int            maxwidth = 0;
-     char       *strvalue;
      int            ljust;
      int            len;
      int            zpad;
--- 151,170 ----

  #define    FMTSTR        1
  #define    FMTNUM        2
! #define    FMTNUM_U    3
! #define    FMTFLOAT    4
! #define    FMTCHAR        5
! #define    FMTWIDTH    6
! #define    FMTLEN        7

  static void
  dopr(char *buffer, const char *format, va_list args, char *end)
  {
      int            ch;
      int            longlongflag = 0;
      int            longflag = 0;
      int            pointflag = 0;
      int            maxwidth = 0;
      int            ljust;
      int            len;
      int            zpad;
***************
*** 173,178 ****
--- 173,179 ----
      const char*        fmtbegin;
      int            fmtpos = 1;
      int            realpos = 0;
+     int            precision;
      int            position;
      char        *output;
      int            percents = 1;
***************
*** 195,200 ****
--- 196,203 ----
          int    pointflag;
          char    func;
          int    realpos;
+         int    longflag;
+         int    longlongflag;
      } *fmtpar, **fmtparptr;

      /* Create enough structures to hold all arguments */
***************
*** 229,240 ****
                  longflag = longlongflag = pointflag = 0;
                  fmtbegin = format - 1;
                  realpos = 0;
!                 position = 0;
          nextch:
                  ch = *format++;
                  switch (ch)
                  {
!                     case 0:
                          goto performpr;
                      case '-':
                          ljust = 1;
--- 232,243 ----
                  longflag = longlongflag = pointflag = 0;
                  fmtbegin = format - 1;
                  realpos = 0;
!                 position = precision = 0;
          nextch:
                  ch = *format++;
                  switch (ch)
                  {
!                     case '\0':
                          goto performpr;
                      case '-':
                          ljust = 1;
***************
*** 251,274 ****
                      case '7':
                      case '8':
                      case '9':
!                         if (pointflag)
!                             /* could also be precision */
!                             maxwidth = maxwidth * 10 + ch - '0';
!                         else
                          {
                              len = len * 10 + ch - '0';
                              position = position * 10 + ch - '0';
                          }
                          goto nextch;
                      case '$':
                          realpos = position;
                          len = 0;
                          goto nextch;
                      case '*':
!                         if (pointflag)
!                             maxwidth = va_arg(args, int);
                          else
!                             len = va_arg(args, int);
                          goto nextch;
                      case '.':
                          pointflag = 1;
--- 254,282 ----
                      case '7':
                      case '8':
                      case '9':
!                         if (!pointflag)
                          {
                              len = len * 10 + ch - '0';
                              position = position * 10 + ch - '0';
                          }
+                         else
+                         {
+                             maxwidth = maxwidth * 10 + ch - '0';
+                             precision = precision * 10 + ch - '0';
+                         }
                          goto nextch;
                      case '$':
                          realpos = position;
                          len = 0;
                          goto nextch;
                      case '*':
!                         MemSet(&fmtpar[fmtpos], 0, sizeof(fmtpar[fmtpos]));
!                         if (!pointflag)
!                             fmtpar[fmtpos].func = FMTLEN;
                          else
!                             fmtpar[fmtpos].func = FMTWIDTH;
!                         fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
!                         fmtpos++;
                          goto nextch;
                      case '.':
                          pointflag = 1;
***************
*** 301,368 ****
  #endif
                      case 'u':
                      case 'U':
!                         /* fmtnum(value,base,dosign,ljust,len,zpad,&output) */
!                         if (longflag)
!                         {
!                             if (longlongflag)
!                                 value = va_arg(args, uint64);
!                             else
!                                 value = va_arg(args, unsigned long);
!                         }
!                         else
!                             value = va_arg(args, unsigned int);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].numvalue = value;
                          fmtpar[fmtpos].base = 10;
                          fmtpar[fmtpos].dosign = 0;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 'o':
                      case 'O':
!                         /* fmtnum(value,base,dosign,ljust,len,zpad,&output) */
!                         if (longflag)
!                         {
!                             if (longlongflag)
!                                 value = va_arg(args, uint64);
!                             else
!                                 value = va_arg(args, unsigned long);
!                         }
!                         else
!                             value = va_arg(args, unsigned int);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].numvalue = value;
                          fmtpar[fmtpos].base = 8;
                          fmtpar[fmtpos].dosign = 0;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 'd':
                      case 'D':
!                         if (longflag)
!                         {
!                             if (longlongflag)
!                             {
!                                 value = va_arg(args, int64);
!                             }
!                             else
!                                 value = va_arg(args, long);
!                         }
!                         else
!                             value = va_arg(args, int);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].numvalue = value;
                          fmtpar[fmtpos].base = 10;
                          fmtpar[fmtpos].dosign = 1;
                          fmtpar[fmtpos].ljust = ljust;
--- 309,348 ----
  #endif
                      case 'u':
                      case 'U':
!                         fmtpar[fmtpos].longflag = longflag;
!                         fmtpar[fmtpos].longlongflag = longlongflag;
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 10;
                          fmtpar[fmtpos].dosign = 0;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM_U;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 'o':
                      case 'O':
!                         fmtpar[fmtpos].longflag = longflag;
!                         fmtpar[fmtpos].longlongflag = longlongflag;
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 8;
                          fmtpar[fmtpos].dosign = 0;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM_U;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 'd':
                      case 'D':
!                         fmtpar[fmtpos].longflag = longflag;
!                         fmtpar[fmtpos].longlongflag = longlongflag;
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 10;
                          fmtpar[fmtpos].dosign = 1;
                          fmtpar[fmtpos].ljust = ljust;
***************
*** 373,444 ****
                          fmtpos++;
                          break;
                      case 'x':
!                         if (longflag)
!                         {
!                             if (longlongflag)
!                                 value = va_arg(args, uint64);
!                             else
!                                 value = va_arg(args, unsigned long);
!                         }
!                         else
!                             value = va_arg(args, unsigned int);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].numvalue = value;
                          fmtpar[fmtpos].base = 16;
                          fmtpar[fmtpos].dosign = 0;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 'X':
!                         if (longflag)
!                         {
!                             if (longlongflag)
!                                 value = va_arg(args, uint64);
!                             else
!                                 value = va_arg(args, unsigned long);
!                         }
!                         else
!                             value = va_arg(args, unsigned int);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].numvalue = value;
                          fmtpar[fmtpos].base = -16;
                          fmtpar[fmtpos].dosign = 1;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 's':
!                         strvalue = va_arg(args, char *);
!                         if (maxwidth > 0 || !pointflag)
!                         {
!                             if (pointflag && len > maxwidth)
!                                 len = maxwidth; /* Adjust padding */
!                             fmtpar[fmtpos].fmtbegin = fmtbegin;
!                             fmtpar[fmtpos].fmtend = format;
!                             fmtpar[fmtpos].value = strvalue;
!                             fmtpar[fmtpos].ljust = ljust;
!                             fmtpar[fmtpos].len = len;
!                             fmtpar[fmtpos].zpad = zpad;
!                             fmtpar[fmtpos].maxwidth = maxwidth;
!                             fmtpar[fmtpos].func = FMTSTR;
!                             fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
!                             fmtpos++;
!                         }
                          break;
                      case 'c':
-                         ch = va_arg(args, int);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].charvalue = ch;
                          fmtpar[fmtpos].func = FMTCHAR;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
--- 353,399 ----
                          fmtpos++;
                          break;
                      case 'x':
!                         fmtpar[fmtpos].longflag = longflag;
!                         fmtpar[fmtpos].longlongflag = longlongflag;
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 16;
                          fmtpar[fmtpos].dosign = 0;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM_U;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 'X':
!                         fmtpar[fmtpos].longflag = longflag;
!                         fmtpar[fmtpos].longlongflag = longlongflag;
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = -16;
                          fmtpar[fmtpos].dosign = 1;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].func = FMTNUM_U;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
                          break;
                      case 's':
!                         fmtpar[fmtpos].fmtbegin = fmtbegin;
!                         fmtpar[fmtpos].fmtend = format;
!                         fmtpar[fmtpos].ljust = ljust;
!                         fmtpar[fmtpos].len = len;
!                         fmtpar[fmtpos].zpad = zpad;
!                         fmtpar[fmtpos].maxwidth = maxwidth;
!                         fmtpar[fmtpos].func = FMTSTR;
!                         fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
!                         fmtpos++;
                          break;
                      case 'c':
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].func = FMTCHAR;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
                          fmtpos++;
***************
*** 448,462 ****
                      case 'f':
                      case 'g':
                      case 'G':
-                         fvalue = va_arg(args, double);
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
-                         fmtpar[fmtpos].fvalue = fvalue;
                          fmtpar[fmtpos].type = ch;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].maxwidth = maxwidth;
!                         fmtpar[fmtpos].precision = position;
                          fmtpar[fmtpos].pointflag = pointflag;
                          fmtpar[fmtpos].func = FMTFLOAT;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
--- 403,415 ----
                      case 'f':
                      case 'g':
                      case 'G':
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].type = ch;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].maxwidth = maxwidth;
!                         fmtpar[fmtpos].precision = precision;
                          fmtpar[fmtpos].pointflag = pointflag;
                          fmtpar[fmtpos].func = FMTFLOAT;
                          fmtpar[fmtpos].realpos = realpos?realpos:fmtpos;
***************
*** 473,494 ****
                  break;
          }
      }
  performpr:
!     /* shuffle pointers */
      for(i = 1; i < fmtpos; i++)
          fmtparptr[i] = &fmtpar[fmtpar[i].realpos];
      output = buffer;
      format = format_save;
      while ((ch = *format++))
      {
          for(i = 1; i < fmtpos; i++)
          {
!             if(ch == '%' && *format == '%')
              {
                  format++;
                  continue;
              }
!             if(fmtpar[i].fmtbegin == format - 1)
              {
                  switch(fmtparptr[i]->func){
                  case FMTSTR:
--- 426,499 ----
                  break;
          }
      }
+
  performpr:
!     /* reorder pointers */
      for(i = 1; i < fmtpos; i++)
          fmtparptr[i] = &fmtpar[fmtpar[i].realpos];
+
+     /* assign values */
+     for(i = 1; i < fmtpos; i++){
+         switch(fmtparptr[i]->func){
+         case FMTSTR:
+             fmtparptr[i]->value = va_arg(args, char *);
+             break;
+         case FMTNUM:
+             if (fmtparptr[i]->longflag)
+             {
+                 if (fmtparptr[i]->longlongflag)
+                     fmtparptr[i]->numvalue = va_arg(args, int64);
+                 else
+                     fmtparptr[i]->numvalue = va_arg(args, long);
+             }
+             else
+                 fmtparptr[i]->numvalue = va_arg(args, int);
+             break;
+         case FMTNUM_U:
+             if (fmtparptr[i]->longflag)
+             {
+                 if (fmtparptr[i]->longlongflag)
+                     fmtparptr[i]->numvalue = va_arg(args, uint64);
+                 else
+                     fmtparptr[i]->numvalue = va_arg(args, unsigned long);
+             }
+             else
+                 fmtparptr[i]->numvalue = va_arg(args, unsigned int);
+             break;
+         case FMTFLOAT:
+             fmtparptr[i]->fvalue = va_arg(args, double);
+             break;
+         case FMTCHAR:
+             fmtparptr[i]->charvalue = va_arg(args, int);
+             break;
+         case FMTLEN:
+             if (i + 1 < fmtpos && fmtpar[i + 1].func != FMTWIDTH)
+                 fmtpar[i + 1].len = va_arg(args, int);
+             /* For "%*.*f", use the second arg */
+             if (i + 2 < fmtpos && fmtpar[i + 1].func == FMTWIDTH)
+                 fmtpar[i + 2].len = va_arg(args, int);
+             break;
+         case FMTWIDTH:
+             if (i + 1 < fmtpos)
+                 fmtpar[i + 1].maxwidth = fmtpar[i + 1].precision =
+                                                         va_arg(args, int);
+             break;
+         }
+     }
+
+     /* do the output */
      output = buffer;
      format = format_save;
      while ((ch = *format++))
      {
          for(i = 1; i < fmtpos; i++)
          {
!             if (ch == '%' && *format == '%')
              {
                  format++;
                  continue;
              }
!             if (fmtpar[i].fmtbegin == format - 1)
              {
                  switch(fmtparptr[i]->func){
                  case FMTSTR:
***************
*** 497,502 ****
--- 502,508 ----
                          fmtparptr[i]->maxwidth, end, &output);
                      break;
                  case FMTNUM:
+                 case FMTNUM_U:
                      fmtnum(fmtparptr[i]->numvalue, fmtparptr[i]->base,
                          fmtparptr[i]->dosign, fmtparptr[i]->ljust,
                          fmtparptr[i]->len, fmtparptr[i]->zpad, end, &output);

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail

From
Nicolai Tufar
Date:
On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:
>
> I have applied a modified version of your patch, attached.

I am so sorry, I sent untested patch again.  Thank you very
much for patience in fixing it. The patch looks perfectly
fine and works under Solaris.

Under win32 I am still struggling with build environment.
In many directories link fails with "undefined reference to
`pg_snprintf'" in other it fails with  "undefined reference to
`_imp__libintl_sprintf'". In yet another directory it fails with
both, like in src/interfaces/ecpg/pgtypeslib:

dlltool --export-all  --output-def pgtypes.def numeric.o datetime.o
common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o
dllwrap  -o libpgtypes.dll --dllname libpgtypes.dll  --def pgtypes.def
numeric.o datetime.o common.o dt_common.o timestamp.o interval.o
pgstrcasecmp.o  -L../../../../src/port -lm
numeric.o(.text+0x19ea):numeric.c: undefined reference to
`_imp__libintl_sprintf'
datetime.o(.text+0x476):datetime.c: undefined reference to `pg_snprintf'
common.o(.text+0x1cd):common.c: undefined reference to `pg_snprintf'
common.o(.text+0x251):common.c: undefined reference to `pg_snprintf'
dt_common.o(.text+0x538):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x553):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x597):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x5d5):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x628):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x7e8):dt_common.c: more undefined references to
`_imp__libintl_sprintf' follow
c:\MinGW\bin\dllwrap.exe: c:\MinGW\bin\gcc exited with status 1
make: *** [libpgtypes.a] Error 1

Could someone with a better grasp of configure and
win32 environment check it? Aparently no one regularily
compiles source code under win32 during development cycle
these days.


Best regards,
Nicolai

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests

From
Bruce Momjian
Date:
Nicolai Tufar wrote:
> On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
> <pgman@candle.pha.pa.us> wrote:
> >
> > I have applied a modified version of your patch, attached.
>

Here is a patch that fixes the %*$ case.

FYI, I am going to pgindent snprintf.c to make it consistent so please
us CVS for your next patch.

I will work on your Win32 compile problem next.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/port/snprintf.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/snprintf.c,v
retrieving revision 1.20
diff -c -c -r1.20 snprintf.c
*** src/port/snprintf.c    16 Mar 2005 06:00:58 -0000    1.20
--- src/port/snprintf.c    16 Mar 2005 14:59:00 -0000
***************
*** 467,481 ****
              fmtparptr[i]->charvalue = va_arg(args, int);
              break;
          case FMTLEN:
!             if (i + 1 < fmtpos && fmtpar[i + 1].func != FMTWIDTH)
!                 fmtpar[i + 1].len = va_arg(args, int);
              /* For "%*.*f", use the second arg */
!             if (i + 2 < fmtpos && fmtpar[i + 1].func == FMTWIDTH)
!                 fmtpar[i + 2].len = va_arg(args, int);
              break;
          case FMTWIDTH:
              if (i + 1 < fmtpos)
!                 fmtpar[i + 1].maxwidth = fmtpar[i + 1].precision =
                                                          va_arg(args, int);
              break;
          }
--- 467,481 ----
              fmtparptr[i]->charvalue = va_arg(args, int);
              break;
          case FMTLEN:
!             if (i + 1 < fmtpos && fmtparptr[i + 1]->func != FMTWIDTH)
!                 fmtparptr[i + 1]->len = va_arg(args, int);
              /* For "%*.*f", use the second arg */
!             if (i + 2 < fmtpos && fmtparptr[i + 1]->func == FMTWIDTH)
!                 fmtparptr[i + 2]->len = va_arg(args, int);
              break;
          case FMTWIDTH:
              if (i + 1 < fmtpos)
!                 fmtparptr[i + 1]->maxwidth = fmtparptr[i + 1]->precision =
                                                          va_arg(args, int);
              break;
          }

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression

From
Bruce Momjian
Date:
Nicolai Tufar wrote:
> On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
> <pgman@candle.pha.pa.us> wrote:
> >
> > I have applied a modified version of your patch, attached.
>
> I am so sorry, I sent untested patch again.  Thank you very
> much for patience in fixing it. The patch looks perfectly
> fine and works under Solaris.
>

Here is another patch that adds sprintf() support, and support for '+',
'h', and fixes '%*s' support.

Applied.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/bin/psql/command.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.141
diff -c -c -r1.141 command.c
*** src/bin/psql/command.c    11 Mar 2005 17:20:34 -0000    1.141
--- src/bin/psql/command.c    16 Mar 2005 21:17:50 -0000
***************
*** 1574,1584 ****
              shellName = DEFAULT_SHELL;

          sys = pg_malloc(strlen(shellName) + 16);
          sprintf(sys,
          /* See EDITOR handling comment for an explaination */
- #ifndef WIN32
                  "exec %s", shellName);
  #else
                  "%s\"%s\"%s", SYSTEMQUOTE, shellName, SYSTEMQUOTE);
  #endif
          result = system(sys);
--- 1574,1586 ----
              shellName = DEFAULT_SHELL;

          sys = pg_malloc(strlen(shellName) + 16);
+ #ifndef WIN32
          sprintf(sys,
          /* See EDITOR handling comment for an explaination */
                  "exec %s", shellName);
  #else
+         sprintf(sys,
+         /* See EDITOR handling comment for an explaination */
                  "%s\"%s\"%s", SYSTEMQUOTE, shellName, SYSTEMQUOTE);
  #endif
          result = system(sys);
Index: src/include/port.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/port.h,v
retrieving revision 1.72
diff -c -c -r1.72 port.h
*** src/include/port.h    11 Mar 2005 19:13:42 -0000    1.72
--- src/include/port.h    16 Mar 2005 21:17:50 -0000
***************
*** 112,117 ****
--- 112,120 ----
  extern int pg_snprintf(char *str, size_t count, const char *fmt,...)
  /* This extension allows gcc to check the format string */
  __attribute__((format(printf, 3, 4)));
+ extern int pg_sprintf(char *str, const char *fmt,...)
+ /* This extension allows gcc to check the format string */
+ __attribute__((format(printf, 2, 3)));
  extern int pg_fprintf(FILE *stream, const char *fmt,...)
  /* This extension allows gcc to check the format string */
  __attribute__((format(printf, 2, 3)));
***************
*** 127,137 ****
--- 130,142 ----
  #ifdef __GNUC__
  #define    vsnprintf(...)    pg_vsnprintf(__VA_ARGS__)
  #define snprintf(...)    pg_snprintf(__VA_ARGS__)
+ #define sprintf(...)    pg_sprintf(__VA_ARGS__)
  #define fprintf(...)    pg_fprintf(__VA_ARGS__)
  #define printf(...)        pg_printf(__VA_ARGS__)
  #else
  #define vsnprintf        pg_vsnprintf
  #define snprintf        pg_snprintf
+ #define sprintf            pg_sprintf
  #define fprintf            pg_fprintf
  #define printf            pg_printf
  #endif
Index: src/port/snprintf.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/snprintf.c,v
retrieving revision 1.22
diff -c -c -r1.22 snprintf.c
*** src/port/snprintf.c    16 Mar 2005 15:12:18 -0000    1.22
--- src/port/snprintf.c    16 Mar 2005 21:17:51 -0000
***************
*** 67,80 ****

  /*static char _id[] = "$PostgreSQL: pgsql/src/port/snprintf.c,v 1.22 2005/03/16 15:12:18 momjian Exp $";*/

- int            pg_snprintf(char *str, size_t count, const char *fmt,...);
- int            pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args);
- int            pg_printf(const char *format,...);
  static void dopr(char *buffer, const char *format, va_list args, char *end);

  /* Prevent recursion */
  #undef    vsnprintf
  #undef    snprintf
  #undef    fprintf
  #undef    printf

--- 67,78 ----

  /*static char _id[] = "$PostgreSQL: pgsql/src/port/snprintf.c,v 1.22 2005/03/16 15:12:18 momjian Exp $";*/

  static void dopr(char *buffer, const char *format, va_list args, char *end);

  /* Prevent recursion */
  #undef    vsnprintf
  #undef    snprintf
+ #undef    sprintf
  #undef    fprintf
  #undef    printf

***************
*** 104,121 ****
  }

  int
  pg_fprintf(FILE *stream, const char *fmt,...)
  {
      int            len;
      va_list        args;
!     char       *buffer[4096];
      char       *p;

      va_start(args, fmt);
!     len = pg_vsnprintf((char *) buffer, (size_t) 4096, fmt, args);
      va_end(args);
!     p = (char *) buffer;
!     for (; *p; p++)
          putc(*p, stream);
      return len;
  }
--- 102,133 ----
  }

  int
+ pg_sprintf(char *str, const char *fmt,...)
+ {
+     int            len;
+     va_list        args;
+     char        buffer[4096];
+
+     va_start(args, fmt);
+     len = pg_vsnprintf(buffer, (size_t) 4096, fmt, args);
+     va_end(args);
+     /* limit output to string */
+     StrNCpy(str, buffer, (len + 1 < 4096) ? len + 1 : 4096);
+     return len;
+ }
+
+ int
  pg_fprintf(FILE *stream, const char *fmt,...)
  {
      int            len;
      va_list        args;
!     char        buffer[4096];
      char       *p;

      va_start(args, fmt);
!     len = pg_vsnprintf(buffer, (size_t) 4096, fmt, args);
      va_end(args);
!     for (p = buffer; *p; p++)
          putc(*p, stream);
      return len;
  }
***************
*** 125,138 ****
  {
      int            len;
      va_list        args;
!     char       *buffer[4096];
      char       *p;

      va_start(args, fmt);
!     len = pg_vsnprintf((char *) buffer, (size_t) 4096, fmt, args);
      va_end(args);
!     p = (char *) buffer;
!     for (; *p; p++)
          putchar(*p);
      return len;
  }
--- 137,150 ----
  {
      int            len;
      va_list        args;
!     char        buffer[4096];
      char       *p;

      va_start(args, fmt);
!     len = pg_vsnprintf(buffer, (size_t) 4096, fmt, args);
      va_end(args);
!
!     for (p = buffer; *p; p++)
          putchar(*p);
      return len;
  }
***************
*** 141,152 ****
   * dopr(): poor man's version of doprintf
   */

! static void fmtstr(char *value, int ljust, int len, int zpad, int maxwidth,
         char *end, char **output);
! static void fmtnum(int64 value, int base, int dosign, int ljust, int len,
!        int zpad, char *end, char **output);
! static void fmtfloat(double value, char type, int ljust, int len,
!          int precision, int pointflag, char *end, char **output);
  static void dostr(char *str, int cut, char *end, char **output);
  static void dopr_outch(int c, char *end, char **output);

--- 153,165 ----
   * dopr(): poor man's version of doprintf
   */

! static void fmtstr(char *value, int ljust, int len, int maxwidth,
         char *end, char **output);
! static void fmtnum(int64 value, int base, int dosign, int forcesign,
!        int ljust, int len, int zpad, char *end, char **output);
! static void fmtfloat(double value, char type, int forcesign,
!        int ljust, int len, int zpad, int precision, int pointflag, char *end,
!        char **output);
  static void dostr(char *str, int cut, char *end, char **output);
  static void dopr_outch(int c, char *end, char **output);

***************
*** 162,174 ****
  dopr(char *buffer, const char *format, va_list args, char *end)
  {
      int            ch;
!     int            longlongflag = 0;
!     int            longflag = 0;
!     int            pointflag = 0;
!     int            maxwidth = 0;
      int            ljust;
      int            len;
      int            zpad;
      int            i;
      const char *format_save;
      const char *fmtbegin;
--- 175,188 ----
  dopr(char *buffer, const char *format, va_list args, char *end)
  {
      int            ch;
!     int            longlongflag;
!     int            longflag;
!     int            pointflag;
!     int            maxwidth;
      int            ljust;
      int            len;
      int            zpad;
+     int            forcesign;
      int            i;
      const char *format_save;
      const char *fmtbegin;
***************
*** 193,198 ****
--- 207,213 ----
          int            maxwidth;
          int            base;
          int            dosign;
+         int            forcesign;
          char        type;
          int            precision;
          int            pointflag;
***************
*** 230,236 ****
          switch (ch)
          {
              case '%':
!                 ljust = len = zpad = maxwidth = 0;
                  longflag = longlongflag = pointflag = 0;
                  fmtbegin = format - 1;
                  realpos = 0;
--- 245,251 ----
          switch (ch)
          {
              case '%':
!                 ljust = len = zpad = forcesign = maxwidth = 0;
                  longflag = longlongflag = pointflag = 0;
                  fmtbegin = format - 1;
                  realpos = 0;
***************
*** 244,249 ****
--- 259,267 ----
                      case '-':
                          ljust = 1;
                          goto nextch;
+                     case '+':
+                         forcesign = 1;
+                         goto nextch;
                      case '0':    /* set zero padding if len not set */
                          if (len == 0 && !pointflag)
                              zpad = '0';
***************
*** 289,294 ****
--- 307,315 ----
                          else
                              longflag = 1;
                          goto nextch;
+                     case 'h':
+                         /* ignore */
+                         goto nextch;
  #ifdef NOT_USED

                          /*
***************
*** 318,323 ****
--- 339,345 ----
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 10;
                          fmtpar[fmtpos].dosign = 0;
+                         fmtpar[fmtpos].forcesign = forcesign;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
***************
*** 333,338 ****
--- 355,361 ----
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 8;
                          fmtpar[fmtpos].dosign = 0;
+                         fmtpar[fmtpos].forcesign = forcesign;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
***************
*** 348,353 ****
--- 371,377 ----
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 10;
                          fmtpar[fmtpos].dosign = 1;
+                         fmtpar[fmtpos].forcesign = forcesign;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
***************
*** 362,367 ****
--- 386,392 ----
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = 16;
                          fmtpar[fmtpos].dosign = 0;
+                         fmtpar[fmtpos].forcesign = forcesign;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
***************
*** 376,381 ****
--- 401,407 ----
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].base = -16;
                          fmtpar[fmtpos].dosign = 1;
+                         fmtpar[fmtpos].forcesign = forcesign;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
                          fmtpar[fmtpos].zpad = zpad;
***************
*** 409,417 ****
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].type = ch;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
!                         fmtpar[fmtpos].maxwidth = maxwidth;
                          fmtpar[fmtpos].precision = precision;
                          fmtpar[fmtpos].pointflag = pointflag;
                          fmtpar[fmtpos].func = FMTFLOAT;
--- 435,444 ----
                          fmtpar[fmtpos].fmtbegin = fmtbegin;
                          fmtpar[fmtpos].fmtend = format;
                          fmtpar[fmtpos].type = ch;
+                         fmtpar[fmtpos].forcesign = forcesign;
                          fmtpar[fmtpos].ljust = ljust;
                          fmtpar[fmtpos].len = len;
!                         fmtpar[fmtpos].zpad = zpad;
                          fmtpar[fmtpos].precision = precision;
                          fmtpar[fmtpos].pointflag = pointflag;
                          fmtpar[fmtpos].func = FMTFLOAT;
***************
*** 504,523 ****
                  {
                      case FMTSTR:
                          fmtstr(fmtparptr[i]->value, fmtparptr[i]->ljust,
!                                fmtparptr[i]->len, fmtparptr[i]->zpad,
!                                fmtparptr[i]->maxwidth, end, &output);
                          break;
                      case FMTNUM:
                      case FMTNUM_U:
                          fmtnum(fmtparptr[i]->numvalue, fmtparptr[i]->base,
!                                fmtparptr[i]->dosign, fmtparptr[i]->ljust,
!                         fmtparptr[i]->len, fmtparptr[i]->zpad, end, &output);
                          break;
                      case FMTFLOAT:
                          fmtfloat(fmtparptr[i]->fvalue, fmtparptr[i]->type,
!                                  fmtparptr[i]->ljust, fmtparptr[i]->len,
!                             fmtparptr[i]->precision, fmtparptr[i]->pointflag,
!                                  end, &output);
                          break;
                      case FMTCHAR:
                          dopr_outch(fmtparptr[i]->charvalue, end, &output);
--- 531,552 ----
                  {
                      case FMTSTR:
                          fmtstr(fmtparptr[i]->value, fmtparptr[i]->ljust,
!                                fmtparptr[i]->len, fmtparptr[i]->maxwidth,
!                                end, &output);
                          break;
                      case FMTNUM:
                      case FMTNUM_U:
                          fmtnum(fmtparptr[i]->numvalue, fmtparptr[i]->base,
!                                fmtparptr[i]->dosign, fmtparptr[i]->forcesign,
!                                fmtparptr[i]->ljust, fmtparptr[i]->len,
!                                fmtparptr[i]->zpad, end, &output);
                          break;
                      case FMTFLOAT:
                          fmtfloat(fmtparptr[i]->fvalue, fmtparptr[i]->type,
!                                fmtparptr[i]->forcesign, fmtparptr[i]->ljust,
!                                fmtparptr[i]->len, fmtparptr[i]->zpad,
!                                fmtparptr[i]->precision, fmtparptr[i]->pointflag,
!                                end, &output);
                          break;
                      case FMTCHAR:
                          dopr_outch(fmtparptr[i]->charvalue, end, &output);
***************
*** 545,562 ****
  }

  static void
! fmtstr(char *value, int ljust, int len, int zpad, int maxwidth, char *end,
         char **output)
  {
      int            padlen,
!                 strlen;            /* amount to pad */

!     if (value == 0)
          value = "<NULL>";
!     for (strlen = 0; value[strlen]; ++strlen);    /* strlen */
!     if (strlen > maxwidth && maxwidth)
!         strlen = maxwidth;
!     padlen = len - strlen;
      if (padlen < 0)
          padlen = 0;
      if (ljust)
--- 574,597 ----
  }

  static void
! fmtstr(char *value, int ljust, int len, int maxwidth, char *end,
         char **output)
  {
      int            padlen,
!                 vallen;            /* amount to pad */

!     if (value == NULL)
          value = "<NULL>";
!     vallen = strlen(value);
!     if (vallen > maxwidth && maxwidth)
!         vallen = maxwidth;
!     if (len < 0)
!     {
!         /* this could happen with a "*" width spec */
!         ljust = 1;
!         len = -len;
!     }
!     padlen = len - vallen;
      if (padlen < 0)
          padlen = 0;
      if (ljust)
***************
*** 575,582 ****
  }

  static void
! fmtnum(int64 value, int base, int dosign, int ljust, int len, int zpad,
!        char *end, char **output)
  {
      int            signvalue = 0;
      uint64        uvalue;
--- 610,617 ----
  }

  static void
! fmtnum(int64 value, int base, int dosign, int forcesign, int ljust,
!        int len, int zpad, char *end, char **output)
  {
      int            signvalue = 0;
      uint64        uvalue;
***************
*** 597,602 ****
--- 632,639 ----
              signvalue = '-';
              uvalue = -value;
          }
+         else if (forcesign)
+             signvalue = '+';
      }
      if (base < 0)
      {
***************
*** 658,676 ****
  }

  static void
! fmtfloat(double value, char type, int ljust, int len, int precision,
!          int pointflag, char *end, char **output)
  {
      char        fmt[32];
      char        convert[512];
      int            padlen = 0;        /* amount to pad */

      /* we rely on regular C library's sprintf to do the basic conversion */
      if (pointflag)
          sprintf(fmt, "%%.%d%c", precision, type);
      else
          sprintf(fmt, "%%%c", type);
!     sprintf(convert, fmt, value);

      if (len < 0)
      {
--- 695,726 ----
  }

  static void
! fmtfloat(double value, char type, int forcesign, int ljust,
!          int len, int zpad, int precision, int pointflag, char *end,
!          char **output)
  {
+     int            signvalue = 0;
+     double        uvalue;
      char        fmt[32];
      char        convert[512];
      int            padlen = 0;        /* amount to pad */

+     uvalue = value;
      /* we rely on regular C library's sprintf to do the basic conversion */
      if (pointflag)
          sprintf(fmt, "%%.%d%c", precision, type);
      else
          sprintf(fmt, "%%%c", type);
!
!     if (value < 0)
!     {
!         signvalue = '-';
!         uvalue = -value;
!     }
!     else if (forcesign)
!         signvalue = '+';
!
!     sprintf(convert, fmt, uvalue);

      if (len < 0)
      {
***************
*** 684,694 ****
--- 734,760 ----
      if (ljust)
          padlen = -padlen;

+     if (zpad && padlen > 0)
+     {
+         if (signvalue)
+         {
+             dopr_outch(signvalue, end, output);
+             --padlen;
+             signvalue = 0;
+         }
+         while (padlen > 0)
+         {
+             dopr_outch(zpad, end, output);
+             --padlen;
+         }
+     }
      while (padlen > 0)
      {
          dopr_outch(' ', end, output);
          --padlen;
      }
+     if (signvalue)
+         dopr_outch(signvalue, end, output);
      dostr(convert, 0, end, output);
      while (padlen < 0)
      {
***************
*** 701,715 ****
  dostr(char *str, int cut, char *end, char **output)
  {
      if (cut)
-     {
          while (*str && cut-- > 0)
              dopr_outch(*str++, end, output);
-     }
      else
-     {
          while (*str)
              dopr_outch(*str++, end, output);
-     }
  }

  static void
--- 767,777 ----