Re: gcc warnings: rx-startindex
On Tue, Jan 15, 2002 at 06:51:25PM -0500, Melvin Smith wrote: Hey Nicholas, Just to be clear, I wasn't directing my concern at anyone, nor am I not glad for the work, heck you've probably contributed more to this project than me. It was just a general concern that I felt should be thought about. I've contributed too high a talk to do ratio than I'd like. And I fear I've upset this Finnish guy by mentioning decaf, so I might have to appease him by contributing more patches to perl5. :-) I was also feeling guilty that I've effectively also suggested that compiler warnings would be a good thing long term, volunteered to clean the current ones up, and then found once compiler warnings were turned on that there are many many of the things, that it's a far bigger job than I'd realised to clean them up (without introducing bugs) and that I'm not going to be able to deliver on cleaning them all up in the short term. (Or my between the lines position of getting to zero warnings and then deciding that all new warnings were introduced by code patches, not my patches to makefiles, so I don't feel duty bound to clean them up, and go off and hack perl5 again instead) I could also be overly paranoid. No no no. There is no such thing as overly paranoid. Even now the bugs are breeding, conspiring, out to get each and every one of us... Nicholas Clark -- ENOJOB http://www.ccl4.org/~nick/CV.html
gcc warnings: rx-startindex
Ok, I've been paging through the hundreds of errors spewn out by gcc
with the new -Wkitchen_sink warnings. Some are pretty clear, but
many others raise questions I'm unsure how to answer.
For example, given the following structure in parrot/rx.h
(note that startindex is unsigned):
typedef struct rxinfo {
STRING *string;
UINTVAL index;
UINTVAL startindex;
BOOLVAL success;
rxflags flags;
UINTVAL minlength;
rxdirection whichway;
PMC *groupstart;
PMC *groupend;
opcode_t *substfunc;
struct Stack_chunk_t* stack;
} rxinfo;
gcc-2.8 rightfully complains that in the following bit of code from
rx.ops, rx-startindex can never be 0.
op rx_advance(in pmc, in int) {
RX_dUNPACK($1);
if(!RxReverse_test(rx)) {
if(++rx-startindex + rx-minlength string_length(rx-string)) {
goto OFFSET($2);
}
}
else {
if(--rx-startindex 0) {
goto OFFSET($2);
}
}
Does anyone know if the correct fix to make startindex signed, eliminate the
unreachable branch, put an assert() in somewhere, ignore the message,
or do something else clever?
--
Andy Dougherty [EMAIL PROTECTED]
Dept. of Physics
Lafayette College, Easton PA 18042
Re: gcc warnings: rx-startindex
You could break it up into:
else if( rx-startindex == 0 ) {
goto OFFSET($2);
}
else {
--rx-startindex
}
- Original Message -
From: Andy Dougherty [EMAIL PROTECTED]
To: Perl6 Internals [EMAIL PROTECTED]
Sent: Tuesday, January 15, 2002 1:47 PM
Subject: gcc warnings: rx-startindex
Ok, I've been paging through the hundreds of errors spewn out by gcc
with the new -Wkitchen_sink warnings. Some are pretty clear, but
many others raise questions I'm unsure how to answer.
For example, given the following structure in parrot/rx.h
(note that startindex is unsigned):
typedef struct rxinfo {
STRING *string;
UINTVAL index;
UINTVAL startindex;
BOOLVAL success;
rxflags flags;
UINTVAL minlength;
rxdirection whichway;
PMC *groupstart;
PMC *groupend;
opcode_t *substfunc;
struct Stack_chunk_t* stack;
} rxinfo;
gcc-2.8 rightfully complains that in the following bit of code from
rx.ops, rx-startindex can never be 0.
op rx_advance(in pmc, in int) {
RX_dUNPACK($1);
if(!RxReverse_test(rx)) {
if(++rx-startindex + rx-minlength string_length(rx-string)) {
goto OFFSET($2);
}
}
else {
if(--rx-startindex 0) {
goto OFFSET($2);
}
}
Does anyone know if the correct fix to make startindex signed, eliminate
the
unreachable branch, put an assert() in somewhere, ignore the message,
or do something else clever?
--
Andy Dougherty [EMAIL PROTECTED]
Dept. of Physics
Lafayette College, Easton PA 18042
Re: gcc warnings: rx-startindex
On Tue, Jan 15, 2002 at 02:06:17PM -0500, Tanton Gibbs wrote:
You could break it up into:
else if( rx-startindex == 0 ) {
goto OFFSET($2);
}
else {
--rx-startindex
}
Or simply change the condition to 'if (rx-startindex-- == 0)'. But
the real question he's asking is: what is correct? Is it better to
leave startindex at zero, or is it ok to let it wrap around? (And if
so, should it really be signed in the first place?) Probably only
Brent Dax can decide.
Re: gcc warnings: rx-startindex
Maybe set the check to :
if(rx-startindex-- == 0)
-Melvin Smith
IBM :: Atlanta Innovation Center
[EMAIL PROTECTED] :: 770-835-6984
Andy Dougherty
doughera@lafayetTo: Perl6 Internals
[EMAIL PROTECTED]
te.edu cc:
Subject: gcc warnings: rx-startindex
01/15/2002 01:47
PM
Ok, I've been paging through the hundreds of errors spewn out by gcc
with the new -Wkitchen_sink warnings. Some are pretty clear, but
many others raise questions I'm unsure how to answer.
For example, given the following structure in parrot/rx.h
(note that startindex is unsigned):
typedef struct rxinfo {
STRING *string;
UINTVAL index;
UINTVAL startindex;
BOOLVAL success;
rxflags flags;
UINTVAL minlength;
rxdirection whichway;
PMC *groupstart;
PMC *groupend;
opcode_t *substfunc;
struct Stack_chunk_t* stack;
} rxinfo;
gcc-2.8 rightfully complains that in the following bit of code from
rx.ops, rx-startindex can never be 0.
op rx_advance(in pmc, in int) {
RX_dUNPACK($1);
if(!RxReverse_test(rx)) {
if(++rx-startindex + rx-minlength string_length(rx-string)) {
goto OFFSET($2);
}
}
else {
if(--rx-startindex 0) {
goto OFFSET($2);
}
}
Does anyone know if the correct fix to make startindex signed, eliminate
the
unreachable branch, put an assert() in somewhere, ignore the message,
or do something else clever?
--
Andy Dougherty[EMAIL PROTECTED]
Dept. of Physics
Lafayette College, Easton PA 18042
Re: gcc warnings: rx-startindex
Eep, you are right, as usual I answered a non-existing question, but
this brings up a point. Various times I've seen people changing
signedness of variables, etc. in one or two places to clear up a
few warnings and I'm wondering how many times there have been ripple
effects.
I'm very happy for all the cleanup work lately, I was just thinking that
removing warnings for the sake of less spam might not be preferable
to leaving the warning in and forcing the originator to rethink his code
in due time.
We are still in an alpha code situation and we realize that things will
probably be rewritten more than once so maybe its something to keep
in mind.
To be clear, what Andy is doing is the right thing(asking what the intent
of a piece of code is), but I doubt everyone does this and I'm sure Dan
doesn't check every single line of every patch before eating each one, or
if
he does he is, as of now, my hero.
-Melvin Smith
IBM :: Atlanta Innovation Center
[EMAIL PROTECTED] :: 770-835-6984
Steve Fink
[EMAIL PROTECTED] To: Tanton Gibbs
[EMAIL PROTECTED]
cc: Andy Dougherty
[EMAIL PROTECTED], Perl6 Internals
01/15/2002 02:26 [EMAIL PROTECTED]
PM Subject: Re: gcc warnings:
rx-startindex
On Tue, Jan 15, 2002 at 02:06:17PM -0500, Tanton Gibbs wrote:
You could break it up into:
else if( rx-startindex == 0 ) {
goto OFFSET($2);
}
else {
--rx-startindex
}
Or simply change the condition to 'if (rx-startindex-- == 0)'. But
the real question he's asking is: what is correct? Is it better to
leave startindex at zero, or is it ok to let it wrap around? (And if
so, should it really be signed in the first place?) Probably only
Brent Dax can decide.
Re: gcc warnings: rx-startindex
On Tue, Jan 15, 2002 at 03:06:45PM -0500, Melvin Smith wrote: To be clear, what Andy is doing is the right thing (asking what the intent of a piece of code is), but I doubt everyone does this and I'm sure Dan doesn't check every single line of every patch before eating each one, or if he does he is, as of now, my hero. Hey, he's already my hero! :-) I'm quite sure he doesn't run make test after every patch, and I doubt he reads every line. Or at least, I doubt he thinks hard about every line. I don't require my heroes to be masochists. And remember, he only signed up to be the internals _designer_!
Re: gcc warnings: rx-startindex
Ok, I take that back, he is my hero too! I used to wonder the same about Linus (Torvalds) when I was on linux-kernel, how he could handle so many patches, hold down a job AND find time to write code of is own is beyond me. Not to mention family, food, fun. -Melvin Smith IBM :: Atlanta Innovation Center [EMAIL PROTECTED] :: 770-835-6984 Steve Fink [EMAIL PROTECTED] To: Melvin Smith/ATLANTA/Contr/IBM@IBMUS cc: Perl6 Internals [EMAIL PROTECTED] 01/15/2002 03:19 Subject: Re: gcc warnings: rx-startindex PM On Tue, Jan 15, 2002 at 03:06:45PM -0500, Melvin Smith wrote: To be clear, what Andy is doing is the right thing (asking what the intent of a piece of code is), but I doubt everyone does this and I'm sure Dan doesn't check every single line of every patch before eating each one, or if he does he is, as of now, my hero. Hey, he's already my hero! :-) I'm quite sure he doesn't run make test after every patch, and I doubt he reads every line. Or at least, I doubt he thinks hard about every line. I don't require my heroes to be masochists. And remember, he only signed up to be the internals _designer_!
Re: gcc warnings: rx-startindex
On Tue, 15 Jan 2002, Melvin Smith wrote: Maybe set the check to : if(rx-startindex-- == 0) That still sets startindex to the equivalent of (unsigned) -1, which might be something like 4294967295. I'm wondering whether that was the actual intent. I suspect probably not. Perhaps Brent really wanted a -1 there and forgot that startindex was unsigned. Perhaps this is supposed to be a Can't happen branch and it would benefit from a stronger diagnostic. I don't understand the surrounding context well enough to say. -- Andy Dougherty [EMAIL PROTECTED] Dept. of Physics Lafayette College, Easton PA 18042
RE: gcc warnings: rx-startindex
Andy Dougherty:
# On Tue, 15 Jan 2002, Melvin Smith wrote:
#
# Maybe set the check to :
#
#if(rx-startindex-- == 0)
#
# That still sets startindex to the equivalent of (unsigned)
# -1, which might
# be something like 4294967295. I'm wondering whether that was
# the actual
# intent. I suspect probably not. Perhaps Brent really wanted
# a -1 there
# and forgot that startindex was unsigned. Perhaps this is
# supposed to be a
# Can't happen branch and it would benefit from a stronger
# diagnostic. I
# don't understand the surrounding context well enough to say.
Short answer: This might as well be signed, and I committed a patch
earlier today that makes it signed.
Long answer:
The structure of that opcode is something like this:
op rx_advance {
if(we're starting at the left side of the string) {
if(++startindex is past the end of the string) {
BACKTRACK
}
}
else { /* we're starting at the right side of the string */
if(--startindex is past the beginning of the string) {
BACKTRACK
}
}
synchronize startindex and index
clear the stack
CONTINUE
}
The --startindex 0 is a convenient way to test if it's past the
beginning. startindex-- == 0 would probably work just as well. Either
way, I've changed startindex to an INTVAL, since more of the contexts
it's used in expect INTVALs than not.
--Brent Dax
[EMAIL PROTECTED]
Parrot Configure pumpking and regex hacker
obra . hawt sysadmin chx0rs
lathos This is sad. I know of *a* hawt sysamin chx0r.
obra I know more than a few.
lathos obra: There are two? Are you sure it's not the same one?
Re: gcc warnings: rx-startindex
On Tue, Jan 15, 2002 at 03:06:45PM -0500, Melvin Smith wrote: Eep, you are right, as usual I answered a non-existing question, but this brings up a point. Various times I've seen people changing signedness of variables, etc. in one or two places to clear up a few warnings and I'm wondering how many times there have been ripple effects. This is a very important point - thanks for bringing it up. It was on my mind yesterday when submitted the patch warnings in classes/, but I should have said it more explicitly - I wasn't going to be able to fix all the warnings within 24 hours because as soon as I started seeing signed/unsigned warnings and comparison with zero always whatever then cleaning up would involve really understanding the code. [actually, it is a very good way to learn what the code is doing] I wouldn't consider me a hero, because all the warnings I did tidy up were simple renaming things (or at least I believe that they were, and if they weren't I didn't afford them enough attention) with the exception of the typedef for the preferef code, where I spent over an hour trying to get my head round just what it was doing before I changed only a few lines. Nicholas Clark -- ENOJOB http://www.ccl4.org/~nick/CV.html
Re: gcc warnings: rx-startindex
At 10:12 PM 1/15/2002 +, Nicholas Clark wrote: On Tue, Jan 15, 2002 at 03:06:45PM -0500, Melvin Smith wrote: Eep, you are right, as usual I answered a non-existing question, but this brings up a point. Various times I've seen people changing signedness of variables, etc. in one or two places to clear up a few warnings and I'm wondering how many times there have been ripple effects. This is a very important point - thanks for bringing it up. It was on my mind yesterday when submitted the patch warnings in classes/, but I should have said it more explicitly - I wasn't going to be able to fix all the warnings within 24 hours because as soon as I started seeing signed/unsigned warnings and comparison with zero always whatever then cleaning up would involve really understanding the code. Hey Nicholas, Just to be clear, I wasn't directing my concern at anyone, nor am I not glad for the work, heck you've probably contributed more to this project than me. It was just a general concern that I felt should be thought about. I could also be overly paranoid. Keep hacking! :) -Melvin
