You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by "Philippe M. Chiasson" <go...@cpan.org> on 2004/02/05 19:52:50 UTC
[mp2 Patch] -Wall -Werrror fixes
Caught a few little problems caught by gcc -Wall -Werror this morning.
Index: src/modules/perl/modperl_const.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_const.c,v
retrieving revision 1.9
diff -u -I$Id -r1.9 modperl_const.c
--- src/modules/perl/modperl_const.c 12 May 2003 13:00:15 -0000 1.9
+++ src/modules/perl/modperl_const.c 5 Feb 2004 18:51:41 -0000
@@ -10,7 +10,7 @@
{
int name_len = strlen(name);
GV **gvp = (GV **)hv_fetch(stash, name, name_len, TRUE);
- SV *val;
+ SV *val = Nullsv;
/* dont redefine */
if (!isGV(*gvp) || !GvCV(*gvp)) {
Index: src/modules/perl/modperl_filter.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_filter.c,v
retrieving revision 1.80
diff -u -I$Id -r1.80 modperl_filter.c
--- src/modules/perl/modperl_filter.c 23 Dec 2003 15:56:01 -0000 1.80
+++ src/modules/perl/modperl_filter.c 5 Feb 2004 18:51:41 -0000
@@ -326,8 +326,8 @@
apr_pool_t *p)
{
char *init_handler_pv_code;
- char *package_name;
- CV *cv;
+ char *package_name = NULL;
+ CV *cv = Nullcv;
MAGIC *mg;
if (handler->mgv_cv) {
Index: xs/APR/PerlIO/apr_perlio.c
===================================================================
RCS file: /home/cvs/modperl-2.0/xs/APR/PerlIO/apr_perlio.c,v
retrieving revision 1.35
diff -u -I$Id -r1.35 apr_perlio.c
--- xs/APR/PerlIO/apr_perlio.c 10 Oct 2003 19:47:12 -0000 1.35
+++ xs/APR/PerlIO/apr_perlio.c 5 Feb 2004 18:51:42 -0000
@@ -58,7 +58,7 @@
SV *arg = (narg > 0) ? *args : PerlIOArg;
PerlIOAPR *st;
const char *path;
- apr_int32_t apr_flag;
+ apr_int32_t apr_flag = APR_READ;
apr_status_t rc;
SV *sv;
@@ -220,7 +220,7 @@
static IV PerlIOAPR_seek(pTHX_ PerlIO *f, Off_t offset, int whence)
{
PerlIOAPR *st = PerlIOSelf(f, PerlIOAPR);
- apr_seek_where_t where;
+ apr_seek_where_t where = APR_SET;
apr_status_t rc;
apr_off_t seek_offset = 0;
@@ -435,7 +435,7 @@
PerlIO *apr_perlio_apr_file_to_PerlIO(pTHX_ apr_file_t *file, apr_pool_t *pool,
apr_perlio_hook_e type)
{
- char *mode;
+ char *mode = "r";
const char *layers = ":APR";
PerlIOAPR *st;
PerlIO *f = PerlIO_allocate(aTHX);
Re: [mp2 Patch] -Wall -Werrror fixes
Posted by Stas Bekman <st...@stason.org>.
Philippe M. Chiasson wrote:
> On Tue, 2004-02-10 at 19:10 -0800, Stas Bekman wrote:
>
>>Philippe M. Chiasson wrote:
>>
>>>Caught a few little problems caught by gcc -Wall -Werror this morning.
>>
>>Good catches, philippe. But plugging these holes with "random" values is ain't
>>right. There is a logic bug that needs to be fixed if we have these issues.
>
>
> Yes, I agree with you. I plugged those holes with pretty bad
> assumptions. I took in your suggestions, and tested it against my gcc
> with success.
>
> Only adding a case default that Perl_croaks works fine too.
Cool. So it's smart enough to figure out that it asserts.
BTW, gcc 3.4 is out.
> With the following patch, compiles cleans & all tests pass!
Looks good, with a tiny remark below:
> Index: src/modules/perl/modperl_filter.c
> ===================================================================
> RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_filter.c,v
> retrieving revision 1.81
> diff -u -I$Id -r1.81 modperl_filter.c
> --- src/modules/perl/modperl_filter.c 9 Feb 2004 19:32:42 -0000 1.81
> +++ src/modules/perl/modperl_filter.c 11 Feb 2004 21:45:37 -0000
> @@ -325,30 +325,28 @@
> int modperl_filter_resolve_init_handler(pTHX_ modperl_handler_t *handler,
> apr_pool_t *p)
> {
> - char *init_handler_pv_code;
> - char *package_name;
> - CV *cv;
> - MAGIC *mg;
> + char *init_handler_pv_code = NULL;
why do you need to init char *?
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [mp2 Patch] -Wall -Werrror fixes
Posted by "Philippe M. Chiasson" <go...@cpan.org>.
On Tue, 2004-02-10 at 19:10 -0800, Stas Bekman wrote:
> Philippe M. Chiasson wrote:
> > Caught a few little problems caught by gcc -Wall -Werror this morning.
>
> Good catches, philippe. But plugging these holes with "random" values is ain't
> right. There is a logic bug that needs to be fixed if we have these issues.
Yes, I agree with you. I plugged those holes with pretty bad
assumptions. I took in your suggestions, and tested it against my gcc
with success.
Only adding a case default that Perl_croaks works fine too.
With the following patch, compiles cleans & all tests pass!
Index: src/modules/perl/modperl_const.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_const.c,v
retrieving revision 1.9
diff -u -I$Id -r1.9 modperl_const.c
--- src/modules/perl/modperl_const.c 12 May 2003 13:00:15 -0000 1.9
+++ src/modules/perl/modperl_const.c 11 Feb 2004 21:45:37 -0000
@@ -4,17 +4,16 @@
typedef SV *(*constants_lookup)(pTHX_ const char *);
typedef const char ** (*constants_group_lookup)(const char *);
-static SV *new_constsub(pTHX_ constants_lookup lookup,
+static void new_constsub(pTHX_ constants_lookup lookup,
HV *caller_stash, HV *stash,
const char *name)
{
int name_len = strlen(name);
GV **gvp = (GV **)hv_fetch(stash, name, name_len, TRUE);
- SV *val;
/* dont redefine */
if (!isGV(*gvp) || !GvCV(*gvp)) {
- val = (*lookup)(aTHX_ name);
+ SV *val = (*lookup)(aTHX_ name);
#if 0
fprintf(stderr, "newCONSTSUB(%s, %s, %d)\n",
@@ -38,8 +37,6 @@
GvCV(alias) = GvCV(*gvp);
}
-
- return val;
}
int modperl_const_compile(pTHX_ const char *classname,
Index: src/modules/perl/modperl_filter.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_filter.c,v
retrieving revision 1.81
diff -u -I$Id -r1.81 modperl_filter.c
--- src/modules/perl/modperl_filter.c 9 Feb 2004 19:32:42 -0000 1.81
+++ src/modules/perl/modperl_filter.c 11 Feb 2004 21:45:37 -0000
@@ -325,30 +325,28 @@
int modperl_filter_resolve_init_handler(pTHX_ modperl_handler_t *handler,
apr_pool_t *p)
{
- char *init_handler_pv_code;
- char *package_name;
- CV *cv;
- MAGIC *mg;
+ char *init_handler_pv_code = NULL;
if (handler->mgv_cv) {
- GV *gv;
- if ((gv = modperl_mgv_lookup(aTHX_ handler->mgv_cv))) {
- cv = modperl_mgv_cv(gv);
- package_name = modperl_mgv_as_string(aTHX_ handler->mgv_cv, p, 1);
- /* fprintf(stderr, "PACKAGE: %s\n", package_name ); */
+ GV *gv = modperl_mgv_lookup(aTHX_ handler->mgv_cv);
+ if (gv) {
+ CV *cv = modperl_mgv_cv(gv);
+ if (cv && SvMAGICAL(cv)) {
+ MAGIC *mg = mg_find((SV*)(cv), '~');
+ init_handler_pv_code = mg ? mg->mg_ptr : NULL;
+ }
+ else {
+ /* XXX: should we complain in such a case? */
+ return 0;
+ }
}
}
-
- if (cv && SvMAGICAL(cv)) {
- mg = mg_find((SV*)(cv), '~');
- init_handler_pv_code = mg ? mg->mg_ptr : NULL;
- }
- else {
- /* XXX: should we complain in such a case? */
- return 0;
- }
if (init_handler_pv_code) {
+ char *package_name =
+ modperl_mgv_as_string(aTHX_ handler->mgv_cv, p, 1);
+ /* fprintf(stderr, "PACKAGE: %s\n", package_name ); */
+
/* eval the code in the parent handler's package's context */
char *code = apr_pstrcat(p, "package ", package_name, ";",
init_handler_pv_code, NULL);
Index: xs/APR/PerlIO/apr_perlio.c
===================================================================
RCS file: /home/cvs/modperl-2.0/xs/APR/PerlIO/apr_perlio.c,v
retrieving revision 1.35
diff -u -I$Id -r1.35 apr_perlio.c
--- xs/APR/PerlIO/apr_perlio.c 10 Oct 2003 19:47:12 -0000 1.35
+++ xs/APR/PerlIO/apr_perlio.c 11 Feb 2004 21:45:37 -0000
@@ -87,6 +87,8 @@
case 'r':
apr_flag = APR_READ;
break;
+ default:
+ Perl_croak(aTHX_ "unknown open mode: %s", mode);
}
/* APR_BINARY: we always do binary read and PerlIO is supposed
@@ -249,6 +251,8 @@
case 2:
where = APR_END;
break;
+ default:
+ Perl_croak(aTHX_ "unknown whence mode: %d", whence);
}
rc = apr_file_seek(st->file, where, &seek_offset);
@@ -451,6 +455,8 @@
case APR_PERLIO_HOOK_READ:
mode = "r";
break;
+ default:
+ Perl_croak(aTHX_ "unknown APR_PERLIO type: %d", type);
};
PerlIO_apply_layers(aTHX_ f, mode, layers);
Re: [mp2 Patch] -Wall -Werrror fixes
Posted by Stas Bekman <st...@stason.org>.
Philippe M. Chiasson wrote:
> Caught a few little problems caught by gcc -Wall -Werror this morning.
Good catches, philippe. But plugging these holes with "random" values is ain't
right. There is a logic bug that needs to be fixed if we have these issues.
> Index: src/modules/perl/modperl_const.c
> ===================================================================
> RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_const.c,v
> retrieving revision 1.9
> diff -u -I$Id -r1.9 modperl_const.c
> --- src/modules/perl/modperl_const.c 12 May 2003 13:00:15 -0000 1.9
> +++ src/modules/perl/modperl_const.c 5 Feb 2004 18:51:41 -0000
> @@ -10,7 +10,7 @@
> {
> int name_len = strlen(name);
> GV **gvp = (GV **)hv_fetch(stash, name, name_len, TRUE);
> - SV *val;
> + SV *val = Nullsv;
>
> /* dont redefine */
> if (!isGV(*gvp) || !GvCV(*gvp)) {
callers of new_constsub never use the return value, so this looks like a more
appropriate patch:
Index: src/modules/perl/modperl_const.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_const.c,v
retrieving revision 1.9
diff -u -r1.9 modperl_const.c
--- src/modules/perl/modperl_const.c 12 May 2003 13:00:15 -0000 1.9
+++ src/modules/perl/modperl_const.c 11 Feb 2004 02:53:24 -0000
@@ -4,9 +4,9 @@
typedef SV *(*constants_lookup)(pTHX_ const char *);
typedef const char ** (*constants_group_lookup)(const char *);
-static SV *new_constsub(pTHX_ constants_lookup lookup,
- HV *caller_stash, HV *stash,
- const char *name)
+static
+void new_constsub(pTHX_ constants_lookup lookup,
+ HV *caller_stash, HV *stash, const char *name)
{
int name_len = strlen(name);
GV **gvp = (GV **)hv_fetch(stash, name, name_len, TRUE);
@@ -38,8 +38,6 @@
GvCV(alias) = GvCV(*gvp);
}
-
- return val;
}
int modperl_const_compile(pTHX_ const char *classname,
> Index: src/modules/perl/modperl_filter.c
> ===================================================================
> RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_filter.c,v
> retrieving revision 1.80
> diff -u -I$Id -r1.80 modperl_filter.c
> --- src/modules/perl/modperl_filter.c 23 Dec 2003 15:56:01 -0000 1.80
> +++ src/modules/perl/modperl_filter.c 5 Feb 2004 18:51:41 -0000
> @@ -326,8 +326,8 @@
> apr_pool_t *p)
> {
> char *init_handler_pv_code;
> - char *package_name;
> - CV *cv;
> + char *package_name = NULL;
> + CV *cv = Nullcv;
> MAGIC *mg;
>
> if (handler->mgv_cv) {
Again, reorganizing the logic removes the need to init vars:
Index: src/modules/perl/modperl_filter.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_filter.c,v
retrieving revision 1.81
diff -u -r1.81 modperl_filter.c
--- src/modules/perl/modperl_filter.c 9 Feb 2004 19:32:42 -0000 1.81
+++ src/modules/perl/modperl_filter.c 11 Feb 2004 03:00:32 -0000
@@ -326,29 +326,27 @@
apr_pool_t *p)
{
char *init_handler_pv_code;
- char *package_name;
- CV *cv;
- MAGIC *mg;
if (handler->mgv_cv) {
- GV *gv;
- if ((gv = modperl_mgv_lookup(aTHX_ handler->mgv_cv))) {
- cv = modperl_mgv_cv(gv);
- package_name = modperl_mgv_as_string(aTHX_ handler->mgv_cv, p, 1);
- /* fprintf(stderr, "PACKAGE: %s\n", package_name ); */
+ GV *gv = modperl_mgv_lookup(aTHX_ handler->mgv_cv);
+ if (gv) {
+ CV *cv = modperl_mgv_cv(gv);
+ if (cv && SvMAGICAL(cv)) {
+ MAGIC *mg = mg_find((SV*)(cv), '~');
+ init_handler_pv_code = mg ? mg->mg_ptr : NULL;
+ }
+ else {
+ /* XXX: should we complain in such a case? */
+ return 0;
+ }
}
}
- if (cv && SvMAGICAL(cv)) {
- mg = mg_find((SV*)(cv), '~');
- init_handler_pv_code = mg ? mg->mg_ptr : NULL;
- }
- else {
- /* XXX: should we complain in such a case? */
- return 0;
- }
-
if (init_handler_pv_code) {
+ char *package_name =
+ modperl_mgv_as_string(aTHX_ handler->mgv_cv, p, 1);
+ /* fprintf(stderr, "PACKAGE: %s\n", package_name ); */
+
/* eval the code in the parent handler's package's context */
char *code = apr_pstrcat(p, "package ", package_name, ";",
init_handler_pv_code, NULL);
> Index: xs/APR/PerlIO/apr_perlio.c
> ===================================================================
> RCS file: /home/cvs/modperl-2.0/xs/APR/PerlIO/apr_perlio.c,v
> retrieving revision 1.35
> diff -u -I$Id -r1.35 apr_perlio.c
> --- xs/APR/PerlIO/apr_perlio.c 10 Oct 2003 19:47:12 -0000 1.35
> +++ xs/APR/PerlIO/apr_perlio.c 5 Feb 2004 18:51:42 -0000
> @@ -58,7 +58,7 @@
> SV *arg = (narg > 0) ? *args : PerlIOArg;
> PerlIOAPR *st;
> const char *path;
> - apr_int32_t apr_flag;
> + apr_int32_t apr_flag = APR_READ;
> apr_status_t rc;
> SV *sv;
>
> @@ -220,7 +220,7 @@
> static IV PerlIOAPR_seek(pTHX_ PerlIO *f, Off_t offset, int whence)
> {
> PerlIOAPR *st = PerlIOSelf(f, PerlIOAPR);
> - apr_seek_where_t where;
> + apr_seek_where_t where = APR_SET;
> apr_status_t rc;
> apr_off_t seek_offset = 0;
>
> @@ -435,7 +435,7 @@
> PerlIO *apr_perlio_apr_file_to_PerlIO(pTHX_ apr_file_t *file, apr_pool_t *pool,
> apr_perlio_hook_e type)
> {
> - char *mode;
> + char *mode = "r";
> const char *layers = ":APR";
> PerlIOAPR *st;
> PerlIO *f = PerlIO_allocate(aTHX);
And again, if we get uninit, it's a problem that shouldn't be masked. the
following covers it:
Can you please try and remove 3 lines with the comment /* make the compiler
happy */, does it realize the Perl_croak is equivalent to return?
Index: xs/APR/PerlIO/apr_perlio.c
===================================================================
RCS file: /home/cvs/modperl-2.0/xs/APR/PerlIO/apr_perlio.c,v
retrieving revision 1.35
diff -u -r1.35 apr_perlio.c
--- xs/APR/PerlIO/apr_perlio.c 10 Oct 2003 19:47:12 -0000 1.35
+++ xs/APR/PerlIO/apr_perlio.c 11 Feb 2004 03:09:02 -0000
@@ -87,6 +87,9 @@
case 'r':
apr_flag = APR_READ;
break;
+ default:
+ apr_flag = APR_READ; /* make the compiler happy */
+ Perl_croak(aTHX_ "unknown open mode: %s", mode);
}
/* APR_BINARY: we always do binary read and PerlIO is supposed
@@ -249,6 +252,9 @@
case 2:
where = APR_END;
break;
+ default:
+ where = APR_CUR; /* make the compiler happy */
+ Perl_croak(aTHX_ "unknown whence mode: %d", whence);
}
rc = apr_file_seek(st->file, where, &seek_offset);
@@ -451,7 +457,10 @@
case APR_PERLIO_HOOK_READ:
mode = "r";
break;
- };
+ default:
+ mode = "r"; /* make the compiler happy */
+ Perl_croak(aTHX_ "unknown whence mode: %d", type);
+ }
PerlIO_apply_layers(aTHX_ f, mode, layers);
if (!f) {
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [mp2 Patch] -Wall -Werrror fixes
Posted by "Philippe M. Chiasson" <go...@cpan.org>.
On Thu, 2004-02-05 at 17:38 -0500, Geoffrey Young wrote:
> Philippe M. Chiasson wrote:
> > Caught a few little problems caught by gcc -Wall -Werror this morning.
> > [...]
> hmm, I'm not seeing any of these with -Wall and -Werror.
> [...]
> my current setup uses MP_MAINTAINER=1 only, which is supposed to enable both
> of those flags.
>
> do you have a compiler flag that I'm missing? I'm on gcc version 3.2.2, so
> maybe you have access to something I don't.
gcc version 3.3.2 20040119 with -Wall -Werror tells me :
modperl_filter.c: In function `modperl_filter_resolve_init_handler':
modperl_filter.c:329: warning: `package_name' might be used uninitialized in this function
modperl_filter.c:330: warning: `cv' might be used uninitialized in this function
modperl_const.c: In function `new_constsub':
modperl_const.c:13: warning: `val' might be used uninitialized in this function
apr_perlio.c: In function `PerlIOAPR_open':
apr_perlio.c:61: warning: `apr_flag' might be used uninitialized in this function
And in the case of apr_perlio, in both instanced I arbitrairly picked a default for
the mode, but instead, it could have a default: clause to the case and warn/bail...
> --Geoff
--
--------------------------------------------------------------------------------
Philippe M. Chiasson /gozer\@(cpan|ectoplasm)\.org/ 88C3A5A5 (122FF51B/C634E37B)
http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3 A5A5
Q: It is impossible to make anything foolproof because fools are so ingenious.
perl -e'$$=\${gozer};{$_=unpack(P7,pack(L,$$));/^JAm_pH\n$/&&print||$$++&&redo}'
Re: [mp2 Patch] -Wall -Werrror fixes
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
Philippe M. Chiasson wrote:
> Caught a few little problems caught by gcc -Wall -Werror this morning.
>
> Index: src/modules/perl/modperl_const.c
> =================================================================== RCS
> file: /home/cvs/modperl-2.0/src/modules/perl/modperl_const.c,v retrieving
> revision 1.9 diff -u -I$Id -r1.9 modperl_const.c ---
> src/modules/perl/modperl_const.c 12 May 2003 13:00:15 -0000 1.9 +++
> src/modules/perl/modperl_const.c 5 Feb 2004 18:51:41 -0000 @@ -10,7 +10,7
> @@ { int name_len = strlen(name); GV **gvp = (GV **)hv_fetch(stash, name,
> name_len, TRUE); - SV *val; + SV *val = Nullsv;
hmm, I'm not seeing any of these with -Wall and -Werror.
cc -I/src/mod_perl-2.0/src/modules/perl -I/src/mod_perl-2.0/xs
-I/apache/2.0/worker/perl-5.8.3/include
-I/apache/2.0/worker/perl-5.8.3/include -D_REENTRANT -D_GNU_SOURCE
-DTHREADS_HAVE_PIDS -DDEBUGGING -fno-strict-aliasing -I/usr/local/include
-I/usr/include/gdbm -I/perl/perl-5.8.3/lib/5.8.3/i686-linux-thread-multi/C
ORE -DMOD_PERL -Wall -Wmissing-prototypes -Wstrict-prototypes
-Wmissing-declarations -Werror -DAP_DEBUG -DAP_HAVE_DESIGNATED_INITIALIZER
-DMP_DEBUG -DMP_TRACE -g -fpic \
-c modperl_const.c && mv modperl_const.o modperl_const.lo
my current setup uses MP_MAINTAINER=1 only, which is supposed to enable both
of those flags.
do you have a compiler flag that I'm missing? I'm on gcc version 3.2.2, so
maybe you have access to something I don't.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org