Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0001644 [Squeak] Kernel minor always 08-10-05 21:26 09-16-05 12:27
Reporter sam View Status public  
Assigned To
Priority normal Resolution no change required  
Status closed   Product Version
Summary 0001644: [ENH] DelayRefactoring-st
Description "Change Set: DelayRefactoring-st
Date: 4 October 2004
Author: Samuel Tardieu

Heavy Delay refactoring to avoid code
duplication. Use idioms such as
#ifEmpty:ifNotEmpty: rather than
#isEmpty #ifTrue:ifFalse:"
Additional Information
Attached Files  DelayRefactoring-st.cs.gz [^] (2,303 bytes) 08-10-05 21:26

- Relationships
related to 0000854closed  Long Delay schedules a deferred image crash 
related to 0001802closed  Add a warning hint to Delay comment 

- Notes
(0002241 - 44 - 66 - 66 - 66 - 66 - 66)
KenCausey
08-10-05 21:28

karl.ramberg@chello.se:

"[er] looks good"
 
(0002242 - 74 - 74 - 74 - 74 - 74 - 74)
KenCausey
08-10-05 21:31

I loaded this into 3.8-6665-basic without errors but did not test further.
 
(0002662 - 41 - 41 - 41 - 41 - 41 - 41)
sam
09-15-05 16:43

Filed as inbox/Kernel-st.37 on Monticello
 
(0002668 - 78 - 78 - 78 - 172 - 172 - 172)
laza
09-16-05 01:35

Haven't yet looked into the refactored code, but how does this relate to 0000854?
 
(0002669 - 16 - 16 - 16 - 16 - 16 - 16)
sam
09-16-05 01:41

It doesn't AFAIK
 
(0002670 - 799 - 821 - 821 - 821 - 821 - 821)
andreas
09-16-05 01:57

Refactoring Delay to "avoid code duplication" is a really BAD idea unless you understand precisely what the impacts of that change are. For beginners, this is THE highest priority code which is run in Squeak, in other words it is time-critical. The speed of this code is critical for accurate responses, it is critical for network services, it affects every last part of the system.

In short: Don't fix it if it ain't broken! This code isn't supposed to be beautiful, it's supposed to be fast! The reason for duplicating code is to make it fast. The reason for not using ifNil:[]ifNotNil:[] is that the compiler may not inline those. Since the effect of these changes are VERY hard to predict I am in favour of leaving things as they are for now unless there is an actual need to change anything.
 
(0002671 - 277 - 301 - 301 - 301 - 301 - 301)
sam
09-16-05 02:02
edited on: 09-16-05 02:03

andreas: makes sense. However, this would deserve at least a note in the class definition.

But as an old-timer in embedded and real-time systems, I am very surprised that Delay code is as critical as you says. I expect most subsystems to be event-driven, not delay-driven.

 
(0002680 - 342 - 354 - 354 - 354 - 354 - 354)
laza
09-16-05 10:28

Once too often I tried myself to beautify some time critical code too just to find out that it runs noticeable slower after my efforts. And I probably do it again next time, just to find out why the things were hacked the way they are. :)

So I think it is ok to resolve this issue, but we could add a comment to Delay to make things clear?
 
(0002682 - 44 - 44 - 44 - 44 - 44 - 44)
MarcusDenker
09-16-05 12:26

cs with waring has been posted in new report
 

- Issue History
Date Modified Username Field Change
08-10-05 21:26 KenCausey New Issue
08-10-05 21:26 KenCausey File Added: DelayRefactoring-st.cs.gz
08-10-05 21:28 KenCausey Note Added: 0002241
08-10-05 21:29 KenCausey Reporter KenCausey => sam
08-10-05 21:31 KenCausey Note Added: 0002242
09-15-05 16:43 sam Note Added: 0002662
09-16-05 01:33 laza Relationship added related to 0000854
09-16-05 01:35 laza Note Added: 0002668
09-16-05 01:41 sam Note Added: 0002669
09-16-05 01:57 andreas Note Added: 0002670
09-16-05 02:02 sam Note Added: 0002671
09-16-05 02:03 sam Note Edited: 0002671
09-16-05 10:28 laza Status new => resolved
09-16-05 10:28 laza Resolution open => no change required
09-16-05 10:28 laza Assigned To  => laza
09-16-05 10:28 laza Note Added: 0002680
09-16-05 10:28 laza Assigned To laza =>
09-16-05 10:48 laza Relationship added related to 0001802
09-16-05 12:26 MarcusDenker Status resolved => closed
09-16-05 12:26 MarcusDenker Note Added: 0002682


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
87 total queries executed.
54 unique queries executed.
Powered by Mantis Bugtracker