I get the following warning compiling CVS HEAD:

[neilc:/Users/neilc/pgsql]% make -C src/backend/utils/error all
[ ... ]
gcc -no-cpp-precomp -O0 -Winline -fno-strict-aliasing -g -Wall 
-Wmissing-prototypes -Wmissing-declarations -I../../../../src/include 
-I/sw/include  -c -o elog.o elog.c -MMD
elog.c: In function `log_line_prefix':
elog.c:1123: warning: passing arg 1 of `localtime' from incompatible 
pointer type

This is on Mac OSX 10.3 w/ gcc 3.3

-Neil


Thu, 18 Mar 2004 15:06:21 -0500
Neil Conway <neilc@samurai.com>
compile warning in CVS HEAD
Neil Conway <neilc@samurai.com> writes:
> I get the following warning compiling CVS HEAD:
> [neilc:/Users/neilc/pgsql]% make -C src/backend/utils/error all
> [ ... ]
> gcc -no-cpp-precomp -O0 -Winline -fno-strict-aliasing -g -Wall 
> -Wmissing-prototypes -Wmissing-declarations -I../../../../src/include 
> -I/sw/include  -c -o elog.o elog.c -MMD
> elog.c: In function `log_line_prefix':
> elog.c:1123: warning: passing arg 1 of `localtime' from incompatible 
> pointer type

Hm, looks like this code incorrectly assumes that the tv_sec field of
struct timeval is necessarily the same datatype as time_t.  I'd suggest
assigning session_start into a local time_t variable.

			regards, tom lane

Thu, 18 Mar 2004 15:29:25 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: compile warning in CVS HEAD 


Tom Lane wrote:

>Neil Conway <neilc@samurai.com> writes:
>  
>
>>I get the following warning compiling CVS HEAD:
>>[neilc:/Users/neilc/pgsql]% make -C src/backend/utils/error all
>>[ ... ]
>>gcc -no-cpp-precomp -O0 -Winline -fno-strict-aliasing -g -Wall 
>>-Wmissing-prototypes -Wmissing-declarations -I../../../../src/include 
>>-I/sw/include  -c -o elog.o elog.c -MMD
>>elog.c: In function `log_line_prefix':
>>elog.c:1123: warning: passing arg 1 of `localtime' from incompatible 
>>pointer type
>>    
>>
>
>Hm, looks like this code incorrectly assumes that the tv_sec field of
>struct timeval is necessarily the same datatype as time_t.  I'd suggest
>assigning session_start into a local time_t variable.
>

*sigh*

my local (linux) man for gettimeofday says this:

       struct timeval {
               time_t         tv_sec;        /* seconds */
               suseconds_t    tv_usec;  /* microseconds */
       };

We could do what you say, or could we just cast it?

cheers

andrew




Thu, 18 Mar 2004 15:43:47 -0500
Andrew Dunstan <andrew@dunslane.net>
Re: compile warning in CVS HEAD
Andrew Dunstan <andrew@dunslane.net> writes:
> *sigh*

> my local (linux) man for gettimeofday says this:

>        struct timeval {
>                time_t         tv_sec;        /* seconds */
>                suseconds_t    tv_usec;  /* microseconds */
>        };

Yeah, but mine (HPUX) says that tv_sec is "unsigned long".  I suspect
that on Darwin the types disagree as to signedness.

> We could do what you say, or could we just cast it?

If they really were different types (as in different widths) then
casting the pointer would be a highly Wrong Thing.  I think copying
to a local is safer, even if it does waste a cycle or two.

			regards, tom lane

Thu, 18 Mar 2004 16:13:14 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: compile warning in CVS HEAD 
Patch attached. Also adds a malloc() check that Neil wanted.

cheers

andrew

Tom Lane wrote:

>Andrew Dunstan <andrew@dunslane.net> writes:
>  
>
>>*sigh*
>>    
>>
>
>  
>
>>my local (linux) man for gettimeofday says this:
>>    
>>
>
>  
>
>>       struct timeval {
>>               time_t         tv_sec;        /* seconds */
>>               suseconds_t    tv_usec;  /* microseconds */
>>       };
>>    
>>
>
>Yeah, but mine (HPUX) says that tv_sec is "unsigned long".  I suspect
>that on Darwin the types disagree as to signedness.
>
>  
>
>>We could do what you say, or could we just cast it?
>>    
>>
>
>If they really were different types (as in different widths) then
>casting the pointer would be a highly Wrong Thing.  I think copying
>to a local is safer, even if it does waste a cycle or two.
>
>			regards, tom lane
>
>---------------------------(end of broadcast)---------------------------
>TIP 6: Have you searched our list archives?
>
>               http://archives.postgresql.org
>
>  
>
Thu, 18 Mar 2004 18:00:58 -0500
Andrew Dunstan <andrew@dunslane.net>
Re: [HACKERS] compile warning in CVS HEAD
Andrew Dunstan <andrew@dunslane.net> writes:
> Patch attached. Also adds a malloc() check that Neil wanted.

Er, what on God's green earth is elog.c doing malloc'ing stuff at all?
This should be a palloc in the ErrorContext; that's what it's for.
As is, this code is guaranteed to fail under out-of-memory conditions.

			regards, tom lane

Thu, 18 Mar 2004 19:35:47 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] compile warning in CVS HEAD 


Tom Lane wrote:

>Andrew Dunstan <andrew@dunslane.net> writes:
>  
>
>>Patch attached. Also adds a malloc() check that Neil wanted.
>>    
>>
>
>Er, what on God's green earth is elog.c doing malloc'ing stuff at all?
>This should be a palloc in the ErrorContext; that's what it's for.
>As is, this code is guaranteed to fail under out-of-memory conditions.
>
>  
>

Well, it's a one-off allocation into a static variable, if that makes 
any difference. If not, I accept the rebuke :-)

I first published the ancestor of this code many months ago, and nobody 
has objected until now. But there is always room for improvement I 
guess, including in my own knowledge. I'll be happy to change it if need be.

cheers

andrew


Thu, 18 Mar 2004 20:06:38 -0500
Andrew Dunstan <andrew@dunslane.net>
Re: [HACKERS] compile warning in CVS HEAD

Where are we on this patch?  There was concern about the malloc() but
that can be fixed.

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

Andrew Dunstan wrote:
> 
> Patch attached. Also adds a malloc() check that Neil wanted.
> 
> cheers
> 
> andrew
> 
> Tom Lane wrote:
> 
> >Andrew Dunstan <andrew@dunslane.net> writes:
> >  
> >
> >>*sigh*
> >>    
> >>
> >
> >  
> >
> >>my local (linux) man for gettimeofday says this:
> >>    
> >>
> >
> >  
> >
> >>       struct timeval {
> >>               time_t         tv_sec;        /* seconds */
> >>               suseconds_t    tv_usec;  /* microseconds */
> >>       };
> >>    
> >>
> >
> >Yeah, but mine (HPUX) says that tv_sec is "unsigned long".  I suspect
> >that on Darwin the types disagree as to signedness.
> >
> >  
> >
> >>We could do what you say, or could we just cast it?
> >>    
> >>
> >
> >If they really were different types (as in different widths) then
> >casting the pointer would be a highly Wrong Thing.  I think copying
> >to a local is safer, even if it does waste a cycle or two.
> >
> >			regards, tom lane
> >
> >---------------------------(end of broadcast)---------------------------
> >TIP 6: Have you searched our list archives?
> >
> >               http://archives.postgresql.org
> >
> >  
> >

> Index: src/backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.128
> diff -c -r1.128 elog.c
> *** src/backend/utils/error/elog.c	15 Mar 2004 15:56:23 -0000	1.128
> --- src/backend/utils/error/elog.c	18 Mar 2004 22:55:44 -0000
> ***************
> *** 1035,1041 ****
> --- 1035,1045 ----
>   	int result_len = 2*NAMEDATALEN + format_len +120 ;
>   
>   	if (result == NULL)
> + 	{
>   		result = malloc(result_len);
> + 		if (result == NULL)
> + 			return "";
> + 	}
>   	result[0] = '\0';
>   
>   	if (format_len > 0)
> ***************
> *** 1119,1126 ****
>   							 localtime(&stamp_time));
>   					break;
>   				case 's':
>   					j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! 							 localtime(&(MyProcPort->session_start.tv_sec)));
>   					break;
>   				case 'i':
>   					j += snprintf(result+j,result_len-j,"%s",
> --- 1123,1131 ----
>   							 localtime(&stamp_time));
>   					break;
>   				case 's':
> + 					stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
>   					j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! 							 localtime(&stamp_time));
>   					break;
>   				case 'i':
>   					j += snprintf(result+j,result_len-j,"%s",

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
>                http://archives.postgresql.org

-- 
  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

Tue, 27 Apr 2004 12:04:55 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] compile warning in CVS HEAD

OK, I talked to Andrew via IM and he says it is fixed in CVS.

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

Andrew Dunstan wrote:
> 
> Patch attached. Also adds a malloc() check that Neil wanted.
> 
> cheers
> 
> andrew
> 
> Tom Lane wrote:
> 
> >Andrew Dunstan <andrew@dunslane.net> writes:
> >  
> >
> >>*sigh*
> >>    
> >>
> >
> >  
> >
> >>my local (linux) man for gettimeofday says this:
> >>    
> >>
> >
> >  
> >
> >>       struct timeval {
> >>               time_t         tv_sec;        /* seconds */
> >>               suseconds_t    tv_usec;  /* microseconds */
> >>       };
> >>    
> >>
> >
> >Yeah, but mine (HPUX) says that tv_sec is "unsigned long".  I suspect
> >that on Darwin the types disagree as to signedness.
> >
> >  
> >
> >>We could do what you say, or could we just cast it?
> >>    
> >>
> >
> >If they really were different types (as in different widths) then
> >casting the pointer would be a highly Wrong Thing.  I think copying
> >to a local is safer, even if it does waste a cycle or two.
> >
> >			regards, tom lane
> >
> >---------------------------(end of broadcast)---------------------------
> >TIP 6: Have you searched our list archives?
> >
> >               http://archives.postgresql.org
> >
> >  
> >

> Index: src/backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.128
> diff -c -r1.128 elog.c
> *** src/backend/utils/error/elog.c	15 Mar 2004 15:56:23 -0000	1.128
> --- src/backend/utils/error/elog.c	18 Mar 2004 22:55:44 -0000
> ***************
> *** 1035,1041 ****
> --- 1035,1045 ----
>   	int result_len = 2*NAMEDATALEN + format_len +120 ;
>   
>   	if (result == NULL)
> + 	{
>   		result = malloc(result_len);
> + 		if (result == NULL)
> + 			return "";
> + 	}
>   	result[0] = '\0';
>   
>   	if (format_len > 0)
> ***************
> *** 1119,1126 ****
>   							 localtime(&stamp_time));
>   					break;
>   				case 's':
>   					j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! 							 localtime(&(MyProcPort->session_start.tv_sec)));
>   					break;
>   				case 'i':
>   					j += snprintf(result+j,result_len-j,"%s",
> --- 1123,1131 ----
>   							 localtime(&stamp_time));
>   					break;
>   				case 's':
> + 					stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
>   					j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! 							 localtime(&stamp_time));
>   					break;
>   				case 'i':
>   					j += snprintf(result+j,result_len-j,"%s",

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
>                http://archives.postgresql.org

-- 
  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

Tue, 27 Apr 2004 12:07:21 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] compile warning in CVS HEAD