www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Length comparison

reply Bill Baxter <wbaxter gmail.com> writes:
This bit me just now:

     if (-1 < somearray.length) {
     }

Ouch.  Took me a while to figure out that that bit of code was the problem.

Is there some way to make these kind of bugs less likely or easier to find?

--bb
Dec 05 2006
parent reply xs0 <xs0 xs0.com> writes:
Bill Baxter wrote:
 This bit me just now:
 
     if (-1 < somearray.length) {
     }
 
 Ouch.  Took me a while to figure out that that bit of code was the problem.
 
 Is there some way to make these kind of bugs less likely or easier to find?

It should be made illegal :) In particular: - comparing a negative constant with an unsigned variable - comparing a too large constant with a signed variable (like 3_000_000_000 and something of type int) - setting an unsigned var to a negative constant (uint a = -1) For other cases, comparing signed with unsigned should produce a warning and, more importantly, work properly: int a; uint b; a < b should be a < 0 || cast(uint)a < b a > b should be a > 0 && cast(uint)a > b etc.etc. I'm quite sure someone will say that's much slower so shouldn't be done, but: - you can do it properly and only compare variables of the same type in the first place - you can trivially make it fast again by casting, with the positive side effect of explicitly stating whether you want to use signed or unsigned comparison - I doubt signed/unsigned comparisons are common enough to even make any noticeable speed difference Thoughts? :) xs0
Dec 05 2006
next sibling parent Lionello Lunesu <lio lunesu.remove.com> writes:
xs0 wrote:
 Bill Baxter wrote:
 This bit me just now:

     if (-1 < somearray.length) {
     }

 Ouch.  Took me a while to figure out that that bit of code was the 
 problem.

 Is there some way to make these kind of bugs less likely or easier to 
 find?

It should be made illegal :) In particular: - comparing a negative constant with an unsigned variable - comparing a too large constant with a signed variable (like 3_000_000_000 and something of type int) - setting an unsigned var to a negative constant (uint a = -1) For other cases, comparing signed with unsigned should produce a warning and, more importantly, work properly: int a; uint b; a < b should be a < 0 || cast(uint)a < b a > b should be a > 0 && cast(uint)a > b etc.etc.

http://d.puremagic.com/issues/show_bug.cgi?id=259 Could you please add your remarks to that bugzilla issue as well? Especially the proper implementation for mixed comparison. I agree that the result should at least be correct, no matter* how slow. L. * of course, within limits :)
Dec 05 2006
prev sibling parent reply Don Clugston <dac nospam.com.au> writes:
xs0 wrote:
 Bill Baxter wrote:
 This bit me just now:

     if (-1 < somearray.length) {
     }

 Ouch.  Took me a while to figure out that that bit of code was the 
 problem.

 Is there some way to make these kind of bugs less likely or easier to 
 find?

It should be made illegal :) In particular: - comparing a negative constant with an unsigned variable - comparing a too large constant with a signed variable (like 3_000_000_000 and something of type int) - setting an unsigned var to a negative constant (uint a = -1) For other cases, comparing signed with unsigned should produce a warning and, more importantly, work properly: int a; uint b; a < b should be a < 0 || cast(uint)a < b a > b should be a > 0 && cast(uint)a > b etc.etc. I'm quite sure someone will say that's much slower so shouldn't be done, but: - you can do it properly and only compare variables of the same type in the first place - you can trivially make it fast again by casting, with the positive side effect of explicitly stating whether you want to use signed or unsigned comparison

But then you have to explicitly specify the type. In your code above, suppose that later on, someone changes 'a' to 'long', and sets a = (1L+int.max)*10 , b=7. Now, the code compiles, but it tells you that a < b !!! You've introduced a bug.
 - I doubt signed/unsigned comparisons are common enough to even make any 
 noticeable speed difference

I think the underlying problem is, there's no way to specify a 'positiveint' type, ie, a number in the range 0..int.max, which can therefore be cast to both int and uint without error. So you have to declare such variables as either 'int' (implying that they can in theory be <0) or as 'uint' (implying that that they can in theory be >int.max). Neither choice is good. There's no way to "do it properly". It's a personal tradeoff which one you use. And for this reason, comparing signed variables with unsigned variables is almost never an error (when people use 'uint', they almost always mean 'positive int'). Detecting mismatch comparisons with literals would be a good thing, though.
Dec 05 2006
parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
Don Clugston wrote:
 xs0 wrote:

 For other cases, comparing signed with unsigned should produce a 
 warning and, more importantly, work properly:

 int a;
 uint b;

 a < b  should be  a < 0 || cast(uint)a < b
 a > b  should be  a > 0 && cast(uint)a > b
 etc.etc.

 I'm quite sure someone will say that's much slower so shouldn't be 
 done, but:
 - you can do it properly and only compare variables of the same type 
 in the first place
 - you can trivially make it fast again by casting, with the positive 
 side effect of explicitly stating whether you want to use signed or 
 unsigned comparison

But then you have to explicitly specify the type. In your code above, suppose that later on, someone changes 'a' to 'long', and sets a = (1L+int.max)*10 , b=7. Now, the code compiles, but it tells you that a < b !!! You've introduced a bug.

I think his point was that the compiler should perform this transformation automatically, i.e. compile "a < b" as if it said "(a < 0 || cast(uint)a < b)". Extrapolating, if 'a' was changed to a long, the compiler should then compile it as if transformed as above, except with the cast changed to "cast(ulong)". So yes, if done manually this introduces a bug, but if that just becomes the new meaning of comparing a signed and unsigned integer then it should work without a problem. I think this is a good idea.
 - I doubt signed/unsigned comparisons are common enough to even make 
 any noticeable speed difference

I think the underlying problem is, there's no way to specify a 'positiveint' type, ie, a number in the range 0..int.max, which can therefore be cast to both int and uint without error.

If opCast could be overloaded this might be possible with a struct, but it wouldn't be pretty (at least, not without also needing implicit casts and overloaded opAssign). But AFAIK as it is, in the current language you're right that this can't be done.
 So you have to declare such variables as either 'int' (implying that 
 they can in theory be <0) or as 'uint' (implying that that they can in 
 theory be >int.max). Neither choice is good. There's no way to "do it 
 properly". It's a personal tradeoff which one you use.
 
 And for this reason, comparing signed variables with unsigned variables 
 is almost never an error (when people use 'uint', they almost always 
 mean 'positive int'). Detecting mismatch comparisons with literals would 
 be a good thing, though.

Yes it would be a good thing if this was detected. IMHO it would be an even better thing if the automatic transformation above would then be performed when such a mismatch is detected :).
Dec 05 2006
parent reply Don Clugston <dac nospam.com.au> writes:
Frits van Bommel wrote:
 Don Clugston wrote:
 xs0 wrote:

 For other cases, comparing signed with unsigned should produce a 
 warning and, more importantly, work properly:

 int a;
 uint b;

 a < b  should be  a < 0 || cast(uint)a < b
 a > b  should be  a > 0 && cast(uint)a > b
 etc.etc.

 I'm quite sure someone will say that's much slower so shouldn't be 
 done, but:
 - you can do it properly and only compare variables of the same type 
 in the first place
 - you can trivially make it fast again by casting, with the positive 
 side effect of explicitly stating whether you want to use signed or 
 unsigned comparison

But then you have to explicitly specify the type. In your code above, suppose that later on, someone changes 'a' to 'long', and sets a = (1L+int.max)*10 , b=7. Now, the code compiles, but it tells you that a < b !!! You've introduced a bug.

I think his point was that the compiler should perform this transformation automatically, i.e. compile "a < b" as if it said "(a < 0 || cast(uint)a < b)". Extrapolating, if 'a' was changed to a long, the compiler should then compile it as if transformed as above, except with the cast changed to "cast(ulong)". So yes, if done manually this introduces a bug, but if that just becomes the new meaning of comparing a signed and unsigned integer then it should work without a problem. I think this is a good idea.
 - I doubt signed/unsigned comparisons are common enough to even make 
 any noticeable speed difference

I think the underlying problem is, there's no way to specify a 'positiveint' type, ie, a number in the range 0..int.max, which can therefore be cast to both int and uint without error.

If opCast could be overloaded this might be possible with a struct, but it wouldn't be pretty (at least, not without also needing implicit casts and overloaded opAssign). But AFAIK as it is, in the current language you're right that this can't be done.
 So you have to declare such variables as either 'int' (implying that 
 they can in theory be <0) or as 'uint' (implying that that they can in 
 theory be >int.max). Neither choice is good. There's no way to "do it 
 properly". It's a personal tradeoff which one you use.

 And for this reason, comparing signed variables with unsigned 
 variables is almost never an error (when people use 'uint', they 
 almost always mean 'positive int'). Detecting mismatch comparisons 
 with literals would be a good thing, though.

Yes it would be a good thing if this was detected. IMHO it would be an even better thing if the automatic transformation above would then be performed when such a mismatch is detected :).

But if a mismatch was detected, would there be any way to request a fast comparison without introducing bugs?
Dec 05 2006
parent Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
Don Clugston wrote:
 Frits van Bommel wrote:
 Don Clugston wrote:


 And for this reason, comparing signed variables with unsigned 
 variables is almost never an error (when people use 'uint', they 
 almost always mean 'positive int'). Detecting mismatch comparisons 
 with literals would be a good thing, though.

Yes it would be a good thing if this was detected. IMHO it would be an even better thing if the automatic transformation above would then be performed when such a mismatch is detected :).

But if a mismatch was detected, would there be any way to request a fast comparison without introducing bugs?

Depends on what you mean by fast and what you mean by bugs ;). Hmm... If "a < 0 || cast(uint)a < b" isn't fast enough for your liking, then probably the only way to make it faster would be to eliminate the "a < 0" part, which would require writing out "cast(uint)a < b" in full. That might introduce bugs if the type of a was changed to something that can have values bigger than an uint can hold. So yeah, that might be a problem. The reality of it is of course that this change probably won't happen anyway, because of Walter's whole "if it looks like C, it behaves like C" philosophy. If he holds to that the best we can probably hope for is some way to express this that doesn't look like C. Maybe some new operators for these "safe comparison" operations... Unless... What does the C standard say about comparing negative signed numbers to unsigned numbers? If the result is Undefined(TM) (or implementation-defined) then defining it in D would not be limited by this :). Forgive me if this is all a bit too much stream-of-consciousness[1], but writing about things is a good way to think about them :). [1]: Is that the right term? Not sure. Not a term I use a lot ;) (especially in English)
Dec 05 2006