Mantis - Squeak
Viewing Issue Advanced Details
2788 Any minor always 02-14-06 08:18 03-28-11 18:27
MarcusDenker  
 
normal  
closed  
fixed  
none    
none 4.2  
0002788: Bug: Use of == for arithmetic equality
From: Dan.Ingalls@Post.Harvard.edu
Subject: Bug: Use of == for arithmetic equality
Date: 13. Februar 2006 18:37:42 GMT+01:00
To: squeak-dev@lists.squeakfoundation.org

I've been playing around with a new VM (heh, heh) which, for a while, happened not to intern (ie force unique instances of) SmallIntegers. In this case the use of == to mean arithmetic equality will not work properly. In my opinion, all such occurrences in the system should be eliminated ASAP; == is not an arithmetic compare in any Smalltalk I know of. While it may work with small constants, it is simply wrong, and an especially bad example for newbies to see. Besides failing in certain interpreters, it will fail in Squeak itself if the integers are not small.

I regret that I don't have time to fix these right now. However, if there is a well-intentioned soul out there, he or she will perhaps find the method below to be quite useful. It found 165 methods in my system with this pattern.

Hope this helps.

    - Dan
-----------------------------------------------

<CompiledMethod>scanForEqSmallConstant
    "Answer whether the receiver contains the pattern <expression> == <constant>,
    where constant is -1, 0, 1, or 2..."

    | scanner |
    scanner _ InstructionStream on: self.
    ^ scanner scanFor: [:instr | (instr between: 116 and: 119) and: [scanner followingByte = 198]]

"
SystemNavigation new browseAllSelect: [:m | m scanForEqSmallConstant]
"

related to 0006721feedback  In 7149 Identity dictionarys sometimes can't find their keys 
 TrueType-fbs.2.mcz [^] (26,597 bytes) 02-14-06 11:25
 Traits-fbs.202.mcz [^] (151,030 bytes) 02-14-06 11:28
 Tools-fbs.55.mcz [^] (444,119 bytes) 02-14-06 11:29
 System-fbs.63.mcz [^] (810,952 bytes) 02-14-06 11:31
 ST80-fbs.28.mcz [^] (312,638 bytes) 02-14-06 11:33
 ReleaseBuilder-fbs.3.mcz [^] (10,310 bytes) 02-14-06 11:35
 PlusTools-fbs.26.mcz [^] (311,992 bytes) 02-14-06 11:37
 Network-fbs.22.mcz [^] (280,224 bytes) 02-14-06 11:39
 Multilingual-fbs.12.mcz [^] (407,756 bytes) 02-14-06 11:40
 Morphic-TrueType-fbs.2.mcz [^] (9,076 bytes) 02-14-06 11:42
 Morphic-Balloon-fbs.2.mcz [^] (15,400 bytes) 02-14-06 11:43
 MorphicExtras-fbs.11.mcz [^] (743,765 bytes) 02-14-06 11:45
 Monticello-fbs.289.mcz [^] (193,904 bytes) 02-14-06 11:53
 KernelTests-fbs.20.mcz [^] (96,209 bytes) 02-14-06 11:54
 Kernel-fbs.89.mcz [^] (542,684 bytes) 02-14-06 11:55
 Graphics-fbs.27.mcz [^] (485,975 bytes) 02-14-06 11:57
 FlexibleVocabularies-fbs.4.mcz [^] (12,217 bytes) 02-14-06 11:57
 Flash-fbs.2.mcz [^] (96,958 bytes) 02-14-06 11:58
 EToys-fbs.7.mcz [^] (638,585 bytes) 02-14-06 11:59
 Compiler-fbs.32.mcz [^] (124,063 bytes) 02-14-06 12:20
 CollectionsTests-fbs.15.mcz [^] (33,149 bytes) 02-14-06 12:21
 Collections-fbs.45.mcz [^] (334,460 bytes) 02-14-06 12:24
 BalloonMMFlash-fbs.4.mcz [^] (939 bytes) 02-14-06 12:26
 Morphic-fbs.66(md.65).mcd [^] (19,570 bytes) 02-14-06 12:33
 IdentityBugs.2.cs [^] (171,793 bytes) 08-09-07 12:06
 7141.sar [^] (48,181 bytes) 11-02-07 23:49
 7142.sar [^] (48,671 bytes) 11-02-07 23:50
 7142-fix.sar [^] (18,988 bytes) 11-03-07 01:52
 7142-timestamp.sar [^] (47,115 bytes) 11-03-07 08:20
 7142-fix-timestamp.sar [^] (18,601 bytes) 11-03-07 08:20

Notes
(0003833)
FrankShearar   
02-14-06 11:26   
The uploaded MCZs contain per-package fixes for this bug report.
(0003836)
FrankShearar   
02-14-06 16:47   
The above MCZs have been/are being copied to the inbox repository, as per Stephane Ducasse's suggestion.
(0003837)
FrankShearar   
02-14-06 17:10   
After adding what new versions I could to the inbox repository, we have:

Packages not in inbox:
Etoys
Monticello
MorphicExtras
PlusTools
Traits

Packages written to inbox:
Collections
CollectionsTests
Compiler
Graphics
Kernel
KernelTests
Morphic
Network
Protocols
SmaCC
ST80
System
Tools

Packages not updated because of potential conflicts:
Multilingual
(0010970)
JTS   
08-09-07 12:14   
This a changeset for 3.10beta (dev image) 7141. I didn't break it into per-package fixes.

(0010992)
edgardec   
08-13-07 12:18   
Reminder sent to: RalphJohnson

As Squeak3.10beta.7141.image this have 172 calls.
I change all and build the apropiate new packages for put on 'http://source.squeakfoundation.org/310' [^] repository
(0011316)
wiz   
10-14-07 05:10   
The fixes in 7158 break project loadBack possibly other things.

3dot10 needs to be reverted and the fixes need to be redone gradually.

Indeed, there need to be tests written for each patch to insure the code works as well after the patch as it did before.

Especially for core patches.

Why not make this report the parent of reports dealling with individual patches?

The other thing it would be worth doing is instead of patching code right off the bat. flag the code that needs patching first. Write tests to insure the patches don't break the code. Introduce the patches gradually rather than massively. When something breaks we can then find it.

The size of this fix made me anxious when I first saw it and I have since found out why. (I've spent a week tracking down a major problem caused by just one method patch.) How may other weeks of work will other patches entail???



(0011330)
nicolas cellier   
10-15-07 22:57   
Isn't it what happened to http://bugs.squeak.org/view.php?id=6711 [^] too ?
(0011334)
RalphJohnson   
10-16-07 12:44   
I went back and ran tests for 7143 and 7138, so I could see what other failures were introduced by this changeset. These changes were installed at 7142.

In addition to the FloatTest failure, there is a failure in WideStringTest>>#testSubstrings and errors in CircleMorphTest and SMDependencyTest.

There is no reason CircleMorphTest should fail, so it is almost certainly caused by this change. However, the test itself does not use =, so it will be a bit of work to track down.

 Odds are good that the others are, too, though we've had other problems with WideString. So, this changeset probably broke four tests.
(0011335)
wiz   
10-16-07 19:39   
Hi Ralph,

I have a question.

The code I looked at had been changed silently. I.E the timestamps were the same as the code replaced. See:
http://209.143.91.36/super/uploads/wiz/fixBugs/ImageSegReload2.jpeg [^]

In the changeset IdentityBugs.2.cs all the code is stamped typically
TFileInOutDescription methodsFor: 'fileIn/Out' stamp: 'JTS 8/9/2007 13:34

So why are the time stamps in the code not JTS also? (it would have made the changes easier to pinpoint?

What did Edgar do to create 7142? Frank's Mcz's (circa Valentines day 2006) or JTS's change set? I am wondering if the right changes were choosen or applied.

This does not change my position on suggesting the fixes need to be made more gradually with test coverage added to insure each step is sound.

But just what fixes were applied to the image???

Yours in curiosity and service, --Jerome Peace
(0011336)
wiz   
10-16-07 19:47   
Reminder sent to: edgardec

Hi edgar,
Can you answer my curiosity as to
which source was used for the changes.
how the changes were made.
and
how it resulted that the timestamps were not changed with the new code?

Thanks --Jer
(0011337)
nicolas cellier   
10-16-07 20:23   
I cannot analyze .mcz because of _ and := appearing as changes...

But I can tell bug there are in IdentityBugs.2.cs.
For example:
    (allSize _ all size) == 0
becoming
    (allSize _ all size) isEmpty
in Behavior>>#inspectAllInstances

It is also changing some Symbol== which is out of scope, will break some optimization, and will make some String equal when they were not.

(0011342)
nicolas cellier   
10-16-07 23:20   
I reviewed some of http://source.squeakfoundation.org/310 [^]

CAUTION PROBABLY A BUG in ImageSegment due to ==endMarker transformed in = at several places (System-edc.106)

Wrong change in St80-edc.40 controller == screenController transformed in =

Wrong change in Morphic-edc.129 ScriptEditorMorph>>updateStatus
  self topEditor==self should not be changed
  ~~ 1 should be changed in ~= 1

Remarks about non arithmetic == converted:
Some Symbol == transformed in =
    arguable. Symbols are known to be unique. == test is faster.
    now, some string will be equal, this was not the case.
Some class == transformed in =
    unnecessary. Harmless?
nil == transformed in =
    arguable: == nil or isNil are valid, fast, and won't fail

You will find these spreaded in CollectionTests-edc.74 EToys-edc.25 Kernel-edc.166 Monticello-edc.313 MorphicExtra-edc.34 System-edc.106 Tools-edc.85

I cannot get version file from Multilingual-edc-29?
(0011343)
edgardec   
10-17-07 10:56   
This happens when you trust Dan ?
No. This happens when you do things the wrong way.
To all, appollogize.
What I do:
Take Dan recipe and change all methods on fly, so no new version.
I don't use any of previous .mcz posted here.
Impossible to coordinate with 3.10 repository.
As I have all my proposal is (for feedback and for any wishing do same)
Take previous ftp image (shoulb be Squeak3.10beta.7137.zip)
Utilities applyUpdatesFromDiskToUpdateNumber: 7141 stopIfGap: false.
Not to do Dan fix for now.
Continue the rest of udates.
(0011348)
nicolas cellier   
10-17-07 23:08   
Additionnal wisdom from Andreas Raab (via v3dot10@lists.squeakfoundation.org):
"it is commonplace to use loops like this:

  obj := 0 someObject.
  [0 == obj] whileFalse:[
    "... do something ..."
    obj := obj nextObject.
  ].
"

So some 0== or ==0 are NOT ARITHMETIC tests and SHOULD NOT be changed.
(otherwise, above loop on objects will exit on first Float = 0.0 encountered).
Whoever is to clean arithmetic tests again has to remind this.
(0011350)
wiz   
10-18-07 05:13   
Hi Edgar, Ralph and Nicholas. JTS and Frank,

It clearly should not be the release teams responsibility to create the changes to fill dan's request for numeric equality rather than identity.

The task can be taken on a little at a time or someone can do a flagging pass. Add flags to all code needing changes w/o changing any codes behavior. Then the flagged code can be attended to piece by piece.

And maybe that code doesn't have the fix applied until it is being worked on for some other urgent reason.

Any code changes need to be REQUIRED to come with tests. At least necessary and sufficient tests. Test that change is necessary because test fails before. Test that change is sufficient because test passes after.

There should also be some stability tests. Test that a behavior works before and after each piecemeal change. For example being able to load back projects with dictionary integrity after code in example I came across was changed.

And of course eyes for code review. Thank you Nicholas.

Cheers --Jer
(0011352)
nicolas cellier   
10-18-07 21:27   
Jerome, your interpretation is quiet conservative.
These changes are not necessary because something is failing. (Unless you play with new VM concepts like Dan).

These changes are necessary because every piece of code in the image is a public example, and using == for testing arithmetic equality is not such a beautiful one. It happens to work in current implementation. But it's not the intent of ==. So it's troubling unecessarily eventual readers.

Also, factoring code to reduce image size does not fit in your model. Removing methods has a lot of value too. Even if it does not solve a bug. Think of it.
(0011412)
matthewf   
11-02-07 23:54   
Uploaded a repackaged version of old change 7142 for easier review. I created 2 sar files containing the changes made in change 7142. 7142.sar contains the changes needed to go from 7141 to 7142, and 7141.sar contains the changes needed to go from 7142 to 7141. I am reviewing the change sets within by simply using a diff utility. Will report back with a review
(0011413)
matthewf   
11-03-07 01:56   
I reviewed all the changes in this fix, and 7142-fix.sar is a zip file with the fixes, one per package. I am pretty sure I found and fixed most bugs introduced by this change, but an independent review would be nice.

I did not modify the timestamps of the methods. If I need to do that, I will.

The sar file applies cleanly to 7158, and has no conflicts. SqueakMap now works again.

(0011414)
wiz   
11-03-07 06:11   
Good grief,
Look how a well intentioned correction has grown into a monster.

Hi Matt.

Thanks for your work on this.

It is my belief that all of the timestamps for the 7142 change must be different from the timestamps of the methods they replaced. Else in the future it will be hard to know if the methods we are looking at are the "blessed" methods or not.

NO method should represent itself as something it is not. And I should have some chance of believing that when two methods have the same time stamp they are the same method. (Domain; the set of all method versions that have made it into a squeak release image. **)

The main thing is by the time we are done with 3dot10 there should be some easy way of distinguishing what was touched from what was not. Compared with say 3dot9 final (7067).

Anyway those are my concerns from a bug tracking point of view.

Again thank you for your ideas and work on this.
(0011415)
matthewf   
11-03-07 08:19   
Uploaded a new version of 7142.sar with the method timestamps changed to 'Equality Fix: edc 8/13/2007 12:18'. This includes exactly the same thing as the 7142 update, except for Semaphore>>critical:ifLocked:, which was changed in a completely different way in 7155.

Also uploaded a new 7142-fix.sar with method timestamps changed to 'mtf 11/3/2007 1:56'

Both apply cleanly to 7158 image from ftp.squeak.org/3.10

To make a 7159 patch that updates all timestamps and fixes broken methods, apply 7142-timestamps.sar then 7142-fix-timestamps.sar to a 7158 image and save

(0011419)
wiz   
11-06-07 06:03   
In
http://lists.squeakfoundation.org/pipermail/squeak-dev/2007-November/122354.html [^]
Subject: cyclic looping with [0 == object] whileFalse: [object := object nextObject]

Tom Phoenix gives a very good use case for testing with 0 == .

I wonder if any of our "fixes" breaks this use case.?

And if a unit test can be devised to check?

Yours in curiosity and service, --Jerome Peace
(0011420)
matthewf   
11-06-07 06:09   
I changed anything that wasn't a size or a list index back to using ==, so I think I fixed it all. Still, an independent review would be nice
(0011421)
matthewf   
11-06-07 06:10   
also, I need to check that the fix applies cleanly on edgars new 7154, since the old 7158 branch seems to be gone
(0011422)
matthewf   
11-06-07 07:16   
I certify that all changes made within 7142-timestamp.sar and 7142-fix-timestamp.sar only make changes that replace = with ==, and vice versa.

I have not re-checked that the changes are correct, but they do not conflict with any changes made from 7141 to 7154, or otherwise change anything other than s/=/==/ or s/==/=/ as appropriate

If you would like to review this change, just unzip 7141.sar and 7142.sar, run them through a diff/merge program (I used vimdiff), and fix everything that needs fixing (easiest to do with an external text editor in this case), and remove all methods that you didn't modify. (for selecting entire methods, the regex '/! !\n\zs' will position the cursor after the end of the current chunk in vim). Do not include the method Semaphore>>critical:ifLocked:, as it conflicts with update 7153
(0012697)
dmason   
09-26-08 16:40   
I'm belated patching these into my image and I happened to note that Monticello-edc.313 has the follow change in ChangeList class>>recentLogOn:startingFrom:

    pos = nil
    pos == nil
        ifTrue: [^ self].
    ^self recent: end - pos on: origChangesFile

I don't know this code, but the "end - pos" suggests that pos is an integer, and the comparison with nil suggests it is a reference to a non-numeric. I would suggest it either say "pos = 0" or "pos == nil", but I may not understand the coding conventions, so feel free to ignore this comment.
(0012699)
wiz   
09-26-08 23:19   
Hi dmason.

The logic is if pos is defined then return a value else do nothing.

The nil is the one unique undefined object so
pos == nil is the appropriate, fastest and most straight forward test for it.

There is a convention (read Kent Becks book) for using a guard clause to return early if the guard conditions are not met.

That is where the ' ifTrue: [ ^ self ] . ' comes from.

If pos passes the guard clause then the logic calls for it to be an integer
so the computed value can be returned.

Ah, I just went back and looked.

The line before your code was

pos := (SelectionMenu labelList: banners selections: positions)
                startUpWithCaption: 'Browse as far back as...'.

Which is a menu selection. Which return one of the menu choices (a positon ) or if the user balked it presumably would return nil to indicate no choice was made.
(0012701)
dmason   
09-28-08 12:14   
Thanks wiz,

That's pretty-much how I understood it. So I think the ChangeList code should be changed to say pos==nil both for code understanding and because it saves a message dispatch (in Object>>=)
(0012706)
wiz   
09-29-08 01:41   
Cool.

Why not start a Mantis report for that issue. We've been OT on this one for the last few comments.

Cheers --Jer
(0014077)
leves   
03-28-11 17:43   
This is fixed in 4.2, but there's no option to indicate that.