XML changes broke assert-enabled vcbuild
The latest set of XML changes (I think latest, at least fairly recent)
broke the win32vc build with asserts enabled. The line:
Assert(fully_escaped || !escape_period);
From what I can tell, this is because the Assert() puts code (the do {}
loop) *before* the declaration of StringInfoData buf, which is not
permitted.
Attached patch seems to fix this. Can someone confirm this is correct
before I put it in?
//Magnus
Attachments:
xml.patchtext/plain; charset=us-asciiDownload
Index: src/backend/utils/adt/xml.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/xml.c,v
retrieving revision 1.27
diff -c -r1.27 xml.c
*** src/backend/utils/adt/xml.c 11 Feb 2007 22:18:15 -0000 1.27
--- src/backend/utils/adt/xml.c 13 Feb 2007 15:27:02 -0000
***************
*** 1324,1335 ****
* SQL/XML doesn't make use of this case anywhere, so it's
* probably a mistake.
*/
- Assert(fully_escaped || !escape_period);
#ifdef USE_LIBXML
StringInfoData buf;
char *p;
initStringInfo(&buf);
for (p = ident; *p; p += pg_mblen(p))
--- 1324,1336 ----
* SQL/XML doesn't make use of this case anywhere, so it's
* probably a mistake.
*/
#ifdef USE_LIBXML
StringInfoData buf;
char *p;
+ Assert(fully_escaped || !escape_period);
+
initStringInfo(&buf);
for (p = ident; *p; p += pg_mblen(p))
On Tue, Feb 13, 2007 at 04:29:16PM +0100, Magnus Hagander wrote:
The latest set of XML changes (I think latest, at least fairly recent)
broke the win32vc build with asserts enabled. The line:
Assert(fully_escaped || !escape_period);From what I can tell, this is because the Assert() puts code (the do {}
loop) *before* the declaration of StringInfoData buf, which is not
permitted.Attached patch seems to fix this. Can someone confirm this is correct
before I put it in?
I just realised I should of course move the comment as well :-) Thus,
the attached patch is more correct.
//Magnus
Attachments:
xml.patchtext/plain; charset=us-asciiDownload
Index: src/backend/utils/adt/xml.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/xml.c,v
retrieving revision 1.27
diff -c -r1.27 xml.c
*** src/backend/utils/adt/xml.c 11 Feb 2007 22:18:15 -0000 1.27
--- src/backend/utils/adt/xml.c 13 Feb 2007 15:38:16 -0000
***************
*** 1320,1335 ****
char *
map_sql_identifier_to_xml_name(char *ident, bool fully_escaped, bool escape_period)
{
/*
* SQL/XML doesn't make use of this case anywhere, so it's
* probably a mistake.
*/
Assert(fully_escaped || !escape_period);
- #ifdef USE_LIBXML
- StringInfoData buf;
- char *p;
-
initStringInfo(&buf);
for (p = ident; *p; p += pg_mblen(p))
--- 1320,1335 ----
char *
map_sql_identifier_to_xml_name(char *ident, bool fully_escaped, bool escape_period)
{
+ #ifdef USE_LIBXML
+ StringInfoData buf;
+ char *p;
+
/*
* SQL/XML doesn't make use of this case anywhere, so it's
* probably a mistake.
*/
Assert(fully_escaped || !escape_period);
initStringInfo(&buf);
for (p = ident; *p; p += pg_mblen(p))
Magnus Hagander wrote:
The latest set of XML changes (I think latest, at least fairly recent)
broke the win32vc build with asserts enabled. The line:
Assert(fully_escaped || !escape_period);From what I can tell, this is because the Assert() puts code (the do {}
loop) *before* the declaration of StringInfoData buf, which is not
permitted.
Certainly.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Magnus Hagander <magnus@hagander.net> writes:
From what I can tell, this is because the Assert() puts code (the do {}
loop) *before* the declaration of StringInfoData buf, which is not
permitted.
This will fail on every ANSI-C compiler, not just vc. Please fix.
regards, tom lane
On Tue, Feb 13, 2007 at 10:50:30AM -0500, Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
From what I can tell, this is because the Assert() puts code (the do {}
loop) *before* the declaration of StringInfoData buf, which is not
permitted.This will fail on every ANSI-C compiler, not just vc. Please fix.
Applied.
//Magnus
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
From what I can tell, this is because the Assert() puts code (the
do {} loop) *before* the declaration of StringInfoData buf, which
is not permitted.This will fail on every ANSI-C compiler, not just vc. Please fix.
We seem to have very poor coverage of such compilers in the build farm,
it seems. Is the vcbuild ready to support a regular build farm run
yet?
It turns out that gcc warns about it anyway. Does anyone have some sort
of clever recipe to catch warnings more easily than by carefully
reading the make output or manually grepping build log files or
something?
--
Peter Eisentraut
http://developer.postgresql.org/~petere/
On Tue, Feb 13, 2007 at 05:23:56PM +0100, Peter Eisentraut wrote:
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
From what I can tell, this is because the Assert() puts code (the
do {} loop) *before* the declaration of StringInfoData buf, which
is not permitted.This will fail on every ANSI-C compiler, not just vc. Please fix.
We seem to have very poor coverage of such compilers in the build farm,
it seems. Is the vcbuild ready to support a regular build farm run
yet?
Backend-wise, yes. It does require some changes to the buildfarm code itself
(can't call make and such), which I beleive Andrew is working on (when
he has free time).
(the vcregress script I just committed was the final "major step"
towards it, I think)
It turns out that gcc warns about it anyway. Does anyone have some sort
of clever recipe to catch warnings more easily than by carefully
reading the make output or manually grepping build log files or
something?
Perhaps something we could have the buildfarm do as well, if it can be
automated?
//Magnus
Peter Eisentraut wrote:
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
From what I can tell, this is because the Assert() puts code (the
do {} loop) *before* the declaration of StringInfoData buf, which
is not permitted.This will fail on every ANSI-C compiler, not just vc. Please fix.
We seem to have very poor coverage of such compilers in the build farm,
it seems. Is the vcbuild ready to support a regular build farm run
yet?It turns out that gcc warns about it anyway. Does anyone have some sort
of clever recipe to catch warnings more easily than by carefully
reading the make output or manually grepping build log files or
something?
Yes, I run /src/tools/pgtest, which shows the warning lines at the end,
after the regression tests are run.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Magnus Hagander <magnus@hagander.net> writes:
On Tue, Feb 13, 2007 at 05:23:56PM +0100, Peter Eisentraut wrote:
It turns out that gcc warns about it anyway. Does anyone have some sort
of clever recipe to catch warnings more easily than by carefully
reading the make output or manually grepping build log files or
something?
Perhaps something we could have the buildfarm do as well, if it can be
automated?
I tend to do "make >make.out 2>make.err" and then look at make.err.
The normal situation with a gcc build is that make.err contains one or
two warnings due to flex's bad habits. We could possibly get that down
to zero if we wanted to work at it. However, most non-gcc compilers
I've looked at generate dozens of mostly-silly warnings, so I'm not sure
if the buildfarm could use this technique or not.
regards, tom lane