Re: [HACKERS] More pgindent follies
[EMAIL PROTECTED] (Nathan Myers) writes: This is good news! Maybe this process can be formalized. That is, each official release migh contain a source file with various modern constructs which we suspect might break old compilers. I have no objection to this, if the process *is* formalized --- that is, we explicitly know and agree to probing for certain obsolescent constructs in each release. The thing that bothered me about this was that pgindent was pushing the envelope without any explicit discussion or advance knowledge. There's plenty of historical cruft in PG that I'd be glad to get rid of, if we can satisfy ourselves that it's no longer needed for any platform of interest. It's stealth obsolescence checks that bother me ;-) After a major release, any modern construct that caused no trouble in the last release is considered OK to use. Probably need to allow a couple major releases, considering that we see lots of people migrating from not-the-last release. But that's a quibble. My point is we need an explicit debate about the risks and benefits of each change. Finding out two years later that a broken tool was doing the experiment without our knowledge is not cool. regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] More pgindent follies
On Wed, May 23, 2001 at 11:58:51AM -0400, Bruce Momjian wrote: I don't see the problem here. My assumption is that the comment is not part of the define, right? Well, that's the question. ANSI C requires comments to be replaced by whitespace before preprocessor commands are detected/executed, but there was an awful lot of variation in preprocessor behavior before ANSI. I suspect there are still preprocessors out there that might misbehave on this input --- for example, by leaving the text * end-of-string */ present in the preprocessor output. Now we still go to considerable lengths to support not-quite-ANSI preprocessors. I don't like the idea that all the work done by configure and c.h in that direction might be wasted because of pgindent carelessness. I agree, but in a certain sense, we would have found those compilers already. This is not new behavour as far as I know, and clearly this would throw a compiler error. This is good news! Maybe this process can be formalized. That is, each official release migh contain a source file with various modern constructs which we suspect might break old compilers. A comment block at the top requests that any breakage be reported. A configure option would allow a user to avoid compiling it, and a comment in the file would explain how to use the option. After a major release, any modern construct that caused no trouble in the last release is considered OK to use. This process makes it easy to leave behind obsolete language restrictions: if you wonder if it's OK now to use a feature that once broke some crufty platform, drop it in modern.c and forget about it. After the next release, you know the answer. Nathan Myers [EMAIL PROTECTED] ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/users-lounge/docs/faq.html
Re: [HACKERS] More pgindent follies
Bruce Momjian [EMAIL PROTECTED] writes: 4. This breaking of a comment attached to a #define scares me. *** *** 1691,1705 #define FIXED_CHAR_SEL 0.04/* about 1/25 */ #define CHAR_RANGE_SEL 0.25 ! #define ANY_CHAR_SEL 0.9 /* not 1, since it won't match end-of-string */ #define FULL_WILDCARD_SEL 5.0 #define PARTIAL_WILDCARD_SEL 2.0 --- 1718,1733 #define FIXED_CHAR_SEL 0.04/* about 1/25 */ #define CHAR_RANGE_SEL 0.25 ! #define ANY_CHAR_SEL 0.9 /* not 1, since it won't match ! * end-of-string */ #define FULL_WILDCARD_SEL 5.0 #define PARTIAL_WILDCARD_SEL 2.0 *** I don't see the problem here. My assumption is that the comment is not part of the define, right? Well, that's the question. ANSI C requires comments to be replaced by whitespace before preprocessor commands are detected/executed, but there was an awful lot of variation in preprocessor behavior before ANSI. I suspect there are still preprocessors out there that might misbehave on this input --- for example, by leaving the text * end-of-string */ present in the preprocessor output. Now we still go to considerable lengths to support not-quite-ANSI preprocessors. I don't like the idea that all the work done by configure and c.h in that direction might be wasted because of pgindent carelessness. regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] More pgindent follies
Bruce Momjian [EMAIL PROTECTED] writes: I agree, but in a certain sense, we would have found those compilers already. This is not new behavour as far as I know, and clearly this would throw a compiler error. [ Digs around ... ] OK, you're right, it's not new behavior; we have instances of similarly-split comments at least back to REL6_5. I guess my fears of portability problems are unfounded. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://www.postgresql.org/search.mpl
[HACKERS] More pgindent follies
As long as you're fixing bugs in pgindent, here are some more follies
exhibited by the most recent pgindent run. Some of these bugs have
been there for awhile, but at least one (the removal of space before
a same-line comment) seems to be new as of the most recent run.
The examples are all taken from
$ cd .../src/backend/utils/adt/
$ cvs diff -c -r1.85 -r1.86 selfuncs.c
but there are many similar cases elsewhere.
1. I can sort of understand inserting space before an #ifdef (though I do
not agree with it), but why before #endif? Why does the first example
insert a blank line *only* before the #endif and not its matching #if?
If you're trying to set off the #if block from surrounding code, seems
like a blank line *after* the #endif would help, not one before. But
none of the here-inserted blank lines seem to me to improve readability;
I'd vote for not making any such changes.
***
*** 648,653
--- 653,659
{
#ifdef NOT_USED /* see neqjoinsel() before
removing me! */
Oid opid = PG_GETARG_OID(0);
+
#endif
Oid relid1 = PG_GETARG_OID(1);
AttrNumber attno1 = PG_GETARG_INT16(2);
***
*** 1078,1087
--- 1091,1102
convert_string_datum(Datum value, Oid typid)
{
char *val;
+
#ifdef USE_LOCALE
char *xfrmstr;
size_t xfrmsize;
size_t xfrmlen;
+
#endif
switch (typid)
***
2. Here are two examples of a long-standing bug: pgindent frequently
(but not always) mis-indents the first 'case' line(s) of a switch.
I don't agree with its indentation of block comments between case
entries, either.
***
*** 845,892
switch (valuetypid)
{
! /*
!* Built-in numeric types
!*/
! case BOOLOID:
! case INT2OID:
! case INT4OID:
! case INT8OID:
! case FLOAT4OID:
! case FLOAT8OID:
! case NUMERICOID:
! case OIDOID:
! case REGPROCOID:
*scaledvalue = convert_numeric_to_scalar(value, valuetypid);
*scaledlobound = convert_numeric_to_scalar(lobound,
boundstypid);
*scaledhibound = convert_numeric_to_scalar(hibound,
boundstypid);
return true;
! /*
!* Built-in string types
!*/
case CHAROID:
case BPCHAROID:
case VARCHAROID:
--- 851,898
switch (valuetypid)
{
! /*
!* Built-in numeric types
!*/
! case BOOLOID:
! case INT2OID:
! case INT4OID:
! case INT8OID:
! case FLOAT4OID:
! case FLOAT8OID:
! case NUMERICOID:
! case OIDOID:
! case REGPROCOID:
*scaledvalue = convert_numeric_to_scalar(value, valuetypid);
*scaledlobound = convert_numeric_to_scalar(lobound,
boundstypid);
*scaledhibound = convert_numeric_to_scalar(hibound,
boundstypid);
return true;
! /*
!* Built-in string types
!*/
case CHAROID:
case BPCHAROID:
case VARCHAROID:
***
*** 911,917
{
switch (typid)
{
! case BOOLOID:
return (double) DatumGetBool(value);
case INT2OID:
return (double) DatumGetInt16(value);
--- 917,923
{
switch (typid)
{
! case BOOLOID:
return (double) DatumGetBool(value);
case INT2OID:
return (double) DatumGetInt16(value);
***
3. This is new misbehavior as of the last pgindent run: whitespace
between a statement and a comment on the same line is sometimes removed.
***
*** 1120,1126
#ifdef USE_LOCALE
/* Guess that transformed string is not much bigger than original */
! xfrmsize = strlen(val) + 32;/* arbitrary pad value here... */
xfrmstr = (char *) palloc(xfrmsize);
xfrmlen = strxfrm(xfrmstr, val, xfrmsize);
if (xfrmlen = xfrmsize)
--- 1137,1143
#ifdef USE_LOCALE
/* Guess that transformed string is not much bigger than original */
! xfrmsize = strlen(val) + 32;/* arbitrary pad value here... */
xfrmstr = (char *) palloc(xfrmsize);
xfrmlen = strxfrm(xfrmstr, val, xfrmsize);
if (xfrmlen
