Обсуждение: Re: [pgsql-hackers-win32] %2$, %1$ gettext placeholder replacement is not working under Win32

>> I don't think we'll hold up release to fix this, but the affected
>> translators may want to think about whether they can avoid
>the problem
>> or not.
>
>Also, it looks like src/port/snprintf.c is not %n$ capable either.
>I'm not sure which platforms that affects.
>
>A possible route to a solution is to upgrade snprintf.c and then use
>it on platforms that don't have this support.  This only fixes those
>cases where we go through snprintf, which is probably not all of the
>affected messages, but it might be enough.  It's not happening for
>8.0.0 though.

Might want to track down which platforms are affected by the file in
port/, and then add win32 to it, and put it up somewhere on a list of
known issues in 8.0?

//Magnus

Greetings,
for a couple of days I have been hacking on src/port/snprintf.c.
With Magnus' help I have managed to implement argument replacement
in snprintf(). The code is very crude and not quite optimised,
any suggestions will be more than welcome.

Here is what I did:

1. I renamed snprintf() to pg_snprintf(), vsnprintf() to pg_vsnprintf()
     and introduced pg_printf() that calls pg_vsnprintf().

2. After running configure I manually added snprintf.o to src/Makefile.global's
   LIBOBJ declaration and -lpgport to Makefile.shlib's DLLWRAP declaration

3. To make sure these functions are used everywhere I introduced the following
   lines at the end of src/include/c.h:

#define snprintf pg_snprintf
#define vsnprintf pg_vsnprintf
#define printf pg_printf

4. I introduced a volatile static char[] variable in snprintf.c code so I can
   grep executables for this string and be sure that it is included.

5. Before running regression test I always ran make install, apparently because
   libpq is read from /usr/local/.

During compilation the following warnings were reported:
../../../src/include/utils/elog.h:121: warning: `pg_printf' is an
unrecognized format function type

which is perfectly fine because we replace printf with pg_printf and
gcc's format() does
know anything about it.

On Linux, PostgreSQL passed regression tests with flying colours and
prints messages
with %n$ just fine. On win32: int8, timestamp, timestamptz, abstime, horology,
constraints, vacuum, and many others failed. To check my code, I
reverted snprintf.c
to the original one from CVS and forced win32 port to use these
functions and it
fails in same places. After examining regression tests diff I came to
conclusion that problem is in fmtnum() function when it operates with
particularly
long integers.

In snprintf() file I changed only and only dopr() function, neve touching
fmtnum(), fmtstr() or fmtfolat().

I would like to kindly ask these questions:

1. Am I on the right to implement %n$ ? Can it be accepted?
2. Why would we not just take FreeBSD's vfprintf() and use it instead?
3. What is wrong with fmtnum() on Win32?
4. What %m format string is used for? And where is it handled. Do I
need to implement it?

I am attaching my version if snprintf.c (because it is too different
from the original to
make a patch) and regression.diff of a failed Win32 regression test
produced wither with
my or with original snprintf.c.


Best regards,
Nicolai Tufar

Nicolai Tufar wrote:
> 1. I renamed snprintf() to pg_snprintf(), vsnprintf() to
> pg_vsnprintf() and introduced pg_printf() that calls pg_vsnprintf().

This is not necessary.  You will notice that we already have configure
tests of snprintf format specifiers (%lld etc.), so using the original
function names is OK even in that case.

> 4. I introduced a volatile static char[] variable in snprintf.c code
> so I can grep executables for this string and be sure that it is
> included.

Remove that when finalizing the code.

> 5. Before running regression test I always ran make install,
> apparently because libpq is read from /usr/local/.

That's because of the -rpath.

> 2. Why would we not just take FreeBSD's vfprintf() and use it
> instead?

Try it.  It's painful.

> 4. What %m format string is used for? And where is it handled. Do I
> need to implement it?

It's only used in the error reporting functions in the server and is
handled there.  You don't need to worry about it.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Sat, 22 Jan 2005 15:31:39 +0100, Peter Eisentraut <peter_e@gmx.net> wrote:
> Nicolai Tufar wrote:
> > 1. I renamed snprintf() to pg_snprintf(), vsnprintf() to
> > pg_vsnprintf() and introduced pg_printf() that calls pg_vsnprintf().
>
> This is not necessary.  You will notice that we already have configure
> tests of snprintf format specifiers (%lld etc.), so using the original
> function names is OK even in that case.

how about a test for the thing like:
printf("%2$s %1$s!\n", "world", "Hello");
It is what I am trying to solve :(

> > 5. Before running regression test I always ran make install,
> > apparently because libpq is read from /usr/local/.
>
> That's because of the -rpath.

I see. Thanks for information.

>
> > 2. Why would we not just take FreeBSD's vfprintf() and use it
> > instead?
>
> Try it.  It's painful.

src/port/snprintf.c is not not a particularly pretty. And
after my rework it got even uglier. I have looked at
FreeBDS's one and it is not *that* complicated. Would
you like me to try to incorporate into PostgreSQL.
NetBSD's one is somewhat simplier but does not
support %n$ feature by the way.

> > 4. What %m format string is used for? And where is it handled. Do I
> > need to implement it?
>
> It's only used in the error reporting functions in the server and is
> handled there.  You don't need to worry about it.

Oh, that is a relief.

> Peter Eisentraut
Nicolai

Nicolai Tufar <ntufar@gmail.com> writes:
> On Sat, 22 Jan 2005 15:31:39 +0100, Peter Eisentraut <peter_e@gmx.net> wrote:
>> Nicolai Tufar wrote:
>>> 2. Why would we not just take FreeBSD's vfprintf() and use it
>>> instead?
>>
>> Try it.  It's painful.

> src/port/snprintf.c is not not a particularly pretty. And
> after my rework it got even uglier. I have looked at
> FreeBDS's one and it is not *that* complicated.

If you can get it to work then it'd be a better bet than writing (and
having to maintain) our own.

            regards, tom lane

On Sat, 22 Jan 2005 17:05:22 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > src/port/snprintf.c is not not a particularly pretty. And
> > after my rework it got even uglier. I have looked at
> > FreeBDS's one and it is not *that* complicated.
>
> If you can get it to work then it'd be a better bet than writing (and
> having to maintain) our own.

Very well, I am starting to work on it.
I will put everything necessary in on file.
So far it is 1500-odd lines and will probably
grow a little bit. Plus we would probably need to
include Double-to-ASCII package too:
http://cvsup.pt.freebsd.org/cgi-bin/cvsweb/cvsweb.cgi/src/contrib/gdtoa/
A daunting task indeed. I will do it but I am afraid
it would not be too portable.

I was wandering if reimplementing print formatting
is the right thing to do.  The problem I have is
in fmtnum() and probably in  fmtfloat() functions
which are very platform-dependent.

The code to shuffle xxprinf() arguments based on
%n$ formatting stirngs is ready why don't shuffle
formatting placeholders too and call OS's vsnprintf()
with modified formatting formatting strings and
va_list?

Or, a similar solution, for every parameter
extract formatting placeholder and value,
call snprintf() and the combine the resulting string?

The first solution requires doing nasty manipulations
with va_list, the second will be slower because of
multiple calls to snprintf().

Which one is better?

Best regards,
Nicolai Tufar


>
>                         regards, tom lane
>

Greetings,

I would like to submit a new version of src/port/snprintf.c
It passes regression tests on Linux and Win32 and
prints all of %n$ messages finely.

I added printf() function too because --help usage-type
output is printed with printf().

I have no experience with autoconf so I would ask you
for help on what changes to do to include libpgport to
src/Makefile.global and  src/Makefile.shlib.

PostgreSQL uses snprintf() in more places than I expected.
So these changes need a through testing. I have no 64-bit
to able to run regression test on it would be nice to run
it on a 64-bit platform too.

Best regards,
Nicolai Tufar