Thread: Compiler warning cleanup - unitilized const variables, pointer type mismatch

I attached another cleanup patch which fixes following warnings reported
by Sun Studio:

"zic.c", line 1534: warning: const object should have initializer: tzh0
"dynloader.c", line 7: warning: empty translation unit
"pgstat.c", line 666: warning: const object should have initializer: all_zeroes
"pgstat.c", line 799: warning: const object should have initializer: all_zeroes
"pgstat.c", line 2552: warning: const object should have initializer: all_zeroes
"preproc.c", line 39569: warning: pointer expression or its operand do not point to the same object yyerror_range,
resultis undefined and non-portable  
"tab-complete.c", line 587: warning: assignment type mismatch:
    pointer to function(pointer to const char, int, int) returning pointer to pointer to char "=" pointer to void


Following list is still unfixed plus see my comments:

"gram.c", line 28487: warning: pointer expression or its operand do not point to the same object yyerror_range, result
isundefined and non-portable  

        - This is really strange warning. The code is really strange
        because it points to -1 index of array, but I'm not bison guru.
        Maybe it is correct, but it would be good if somebody check it.

"../../../src/include/pg_config.h", line 782: warning: macro redefined: _FILE_OFFSET_BITS

   - This probably needs some extra love in configure.

"regc_lex.c", line 401: warning: loop not entered at top
"regc_lex.c", line 484: warning: loop not entered at top
"regc_lex.c", line 578: warning: loop not entered at top
"regc_lex.c", line 610: warning: loop not entered at top
"regc_lex.c", line 870: warning: loop not entered at top
"regc_lex.c", line 1073: warning: loop not entered at top
"postgres.c", line 3845: warning: loop not entered at top

        - Assert on not reached place probably confuse compiler. I'm not
        sure if it make sense nowadays? Most compiler should optimize
        this anyway and code is removed. I suppose to remove these
        asserts.


    Zdenek

Attachment
On Thu, May 28, 2009 at 11:11:20AM +0200, Zdenek Kotala wrote:
> I attached another cleanup patch which fixes following warnings reported
> by Sun Studio:
> ...
> "preproc.c", line 39569: warning: pointer expression or its operand do not point to the same object yyerror_range,
resultis undefined and non-portable 
 
> ...
> Following list is still unfixed plus see my comments:
> 
> "gram.c", line 28487: warning: pointer expression or its operand do not point to the same object yyerror_range,
resultis undefined and non-portable 
 
> ...

These two should be the same, both coming from bison. Both files are
auto-generated, thus it might be bison that has to be fixed to remove this
warning. Given that I didn't find any mentioning of preproc in your patch I
suppose it just hit the wrong list though.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!


Michael Meskes píše v čt 28. 05. 2009 v 13:33 +0200:
> On Thu, May 28, 2009 at 11:11:20AM +0200, Zdenek Kotala wrote:
> > I attached another cleanup patch which fixes following warnings reported
> > by Sun Studio:
> > ...
> > "preproc.c", line 39569: warning: pointer expression or its operand do not point to the same object yyerror_range,
resultis undefined and non-portable 
 
> > ...
> > Following list is still unfixed plus see my comments:
> > 
> > "gram.c", line 28487: warning: pointer expression or its operand do not point to the same object yyerror_range,
resultis undefined and non-portable 
 
> > ...
> 
> These two should be the same, both coming from bison. Both files are
> auto-generated, thus it might be bison that has to be fixed to remove this
> warning. 

yeah it is generated, but question is if generated code is valid or it
is bug in bison. If it bison bug we need to care about it. There is the
code:
 yyerror_range[1] = yylloc; /* Using YYLLOC is tempting, but would change the location of    the look-ahead.  YYLOC is
availablethough.  */ YYLLOC_DEFAULT (yyloc, (yyerror_range - 1), 2); *++yylsp = yyloc;
 

Problem is with YYLLOC_DEFAULT. When I look on macro definition 

#define YYLLOC_DEFAULT(Current, Rhs, N)          \ Current.first_line   = Rhs[1].first_line;      \
Current.first_column= Rhs[1].first_column;    \ Current.last_line    = Rhs[N].last_line;       \ Current.last_column  =
Rhs[N].last_column;

It seems to me that it is OK, because 1 is used as a index which finally
point on yyerror_range[0]. 


> Given that I didn't find any mentioning of preproc in your patch I
> suppose it just hit the wrong list though.

I'm sorry copy paste error. Yeah, I did not fix preproc too.
    Zdenek




On Thu, May 28, 2009 at 01:51:07PM +0200, Zdenek Kotala wrote:
> Problem is with YYLLOC_DEFAULT. When I look on macro definition 
> 
> #define YYLLOC_DEFAULT(Current, Rhs, N)          \
>   Current.first_line   = Rhs[1].first_line;      \
>   Current.first_column = Rhs[1].first_column;    \
>   Current.last_line    = Rhs[N].last_line;       \
>   Current.last_column  = Rhs[N].last_column;
> 
> It seems to me that it is OK, because 1 is used as a index which finally
> point on yyerror_range[0]. 

Wait, this is the bison definition. Well to be more precise the bison
definition in your bison version. Mine is different:

# define YYLLOC_DEFAULT(Current, Rhs, N)                                \   do
                       \     if (YYID (N))                                                    \       {
                                             \         (Current).first_line   = YYRHSLOC (Rhs, 1).first_line;        \
      (Current).first_column = YYRHSLOC (Rhs, 1).first_column;      \         (Current).last_line    = YYRHSLOC (Rhs,
N).last_line;        \         (Current).last_column  = YYRHSLOC (Rhs, N).last_column;       \       }
                                            \     else                                                              \
   {                                                               \         (Current).first_line   =
(Current).last_line  =              \           YYRHSLOC (Rhs, 0).last_line;                                \
(Current).first_column= (Current).last_column =              \           YYRHSLOC (Rhs, 0).last_column;
            \      }                                                               \   while (YYID (0))
 

Having said that, it doesn't really matter as we redefine the macro:

#define YYLLOC_DEFAULT(Current, Rhs, N) \       do { \               if (N) \                       (Current) =
(Rhs)[1];\               else \                       (Current) = (Rhs)[0]; \       } while (0)
 

I have to admit that those version look strikingly unsimilar to me. There is no
reference to Rhs[N] in our macro at all. But then I have no idea whether this
is needed.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!


Michael Meskes píše v čt 28. 05. 2009 v 14:47 +0200:
> On Thu, May 28, 2009 at 01:51:07PM +0200, Zdenek Kotala wrote:
> > Problem is with YYLLOC_DEFAULT. When I look on macro definition 
> > 
> > #define YYLLOC_DEFAULT(Current, Rhs, N)          \
> >   Current.first_line   = Rhs[1].first_line;      \
> >   Current.first_column = Rhs[1].first_column;    \
> >   Current.last_line    = Rhs[N].last_line;       \
> >   Current.last_column  = Rhs[N].last_column;
> > 
> > It seems to me that it is OK, because 1 is used as a index which finally
> > point on yyerror_range[0]. 
> 
> Wait, this is the bison definition. Well to be more precise the bison
> definition in your bison version. Mine is different:

I took it from documentation. I have same as you in generated code.

> # define YYLLOC_DEFAULT(Current, Rhs, N)                                \
>     do                                                                  \
>       if (YYID (N))                                                    \
>         {                                                               \
>           (Current).first_line   = YYRHSLOC (Rhs, 1).first_line;        \
>           (Current).first_column = YYRHSLOC (Rhs, 1).first_column;      \
>           (Current).last_line    = YYRHSLOC (Rhs, N).last_line;         \
>           (Current).last_column  = YYRHSLOC (Rhs, N).last_column;       \
>         }                                                               \
>       else                                                              \
>         {                                                               \
>           (Current).first_line   = (Current).last_line   =              \
>             YYRHSLOC (Rhs, 0).last_line;                                \
>           (Current).first_column = (Current).last_column =              \
>             YYRHSLOC (Rhs, 0).last_column;                              \
>        }                                                               \
>     while (YYID (0))
> 
> Having said that, it doesn't really matter as we redefine the macro:
> 
> #define YYLLOC_DEFAULT(Current, Rhs, N) \
>         do { \
>                 if (N) \
>                         (Current) = (Rhs)[1]; \
>                 else \
>                         (Current) = (Rhs)[0]; \
>         } while (0)
> 
> I have to admit that those version look strikingly unsimilar to me. There is no
> reference to Rhs[N] in our macro at all. But then I have no idea whether this
> is needed.

Current is only int. See gramparse.h. I think we could rewrite it this
way:

#define YYLLOC_DEFAULT(Current, Rhs, N) \do { \    if (N) \        (Current) = (Rhs)[1]; \    else \        (Current) =
(Rhs)[N];\} while (0)
 

It is same result and compiler is quite.
    Zdenek







Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> I attached another cleanup patch which fixes following warnings reported
> by Sun Studio:

I'm not too impressed with any of these.  The proposed added
initializers just increase future maintenance effort without solving
any real problem (since the variables are required by C standard to
initialize to zero).  The proposed signature change on psql_completion
is going to replace a warning on your system with outright failures on
other people's.
        regards, tom lane


Michael Meskes <meskes@postgresql.org> writes:
> I have to admit that those version look strikingly unsimilar to me. There is no
> reference to Rhs[N] in our macro at all. But then I have no idea whether this
> is needed.

The default version of the macro is intended to track both the starting
and ending locations of every construct.  Our simplified version only
tracks the starting locations.  The inputs are RHS[k], the location
values for the constituent elements of the current production, and
the output is the location value for the construct being formed.
In the default version, you naturally want to copy the start of
RHS[1] and the end of RHS[N], where N is the number of production
elements.  In ours, we just copy the location of RHS[1].  However,
both macros need a special case for empty productions (with N = 0).

AFAICS, Sun's compiler is just too stupid and shouldn't be emitting
this warning.  Perhaps the right response is to file a bug report
against the compiler.
        regards, tom lane


Tom Lane píše v čt 28. 05. 2009 v 11:42 -0400:
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> > I attached another cleanup patch which fixes following warnings reported
> > by Sun Studio:
> 
> I'm not too impressed with any of these.  The proposed added
> initializers just increase future maintenance effort without solving
> any real problem (since the variables are required by C standard to
> initialize to zero). 

Agree.

>  The proposed signature change on psql_completion
> is going to replace a warning on your system with outright failures on
> other people's.

I check readline and definition is still same at least from 5.0 version.
I'm not still understand why it should failure on other systems. I
looked on revision 1.30 of the file and there is only readline-4.2
support mentioned. Is readline 4.2 the problem?
Zdenek



Tom Lane píše v čt 28. 05. 2009 v 11:57 -0400:
> ).
> 
> AFAICS, Sun's compiler is just too stupid and shouldn't be emitting
> this warning.  Perhaps the right response is to file a bug report
> against the compiler.

I checked it and it is already know bug. It is new lint style check in
Sun Studio 12. Unfortunately, problem is that current workflow does not
allow to detect if code is dead or not in the verification phase. Next
sun studio release could fix it.
Zdenek



Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> Tom Lane píše v čt 28. 05. 2009 v 11:42 -0400:
>> The proposed signature change on psql_completion
>> is going to replace a warning on your system with outright failures on
>> other people's.

> I check readline and definition is still same at least from 5.0 version.
> I'm not still understand why it should failure on other systems. I
> looked on revision 1.30 of the file and there is only readline-4.2
> support mentioned. Is readline 4.2 the problem?

[ pokes around... ]  Actually I think the reason it's like this is that
very old versions of readline have completion_matches() rather than
rl_completion_matches(), and the former is declared to take char * not
const char *.  So it still would compile, you'd just get cast-away-const
warnings.  Which is probably okay considering that hardly anyone is
likely to still be using such old readline libs anyway.

We could try experimenting with that after we branch for 8.5.  I'm not
eager to fool with it in late beta, as we'd be invalidating any port
testing already done.
        regards, tom lane