[fpc-devel] Re: Graph unit under Go32V2

[borsa77 at libero.it]

On 11 Mar 07, at 21:57, Tomas Hajny wrote: 

>1) Please, always create diffs against the trunk 
>version, not 2.0.x. Merging fixes to 2.0.x may be 
>done afterwards if needed, but not the other way 
>round. 

Yes, I will do. 

>2) At least some of your changes related to 
>saving of registers shouldn't be needed any 
>longer - the reason is that there was a problem 
>in the compiler which resulted in incorrect 
>assumptions regarding saved registers (which in 
>turn resulted in the crash as observed with Graph 
>in 2.0.4). However, this issue has been fixed in 
>compiler in the meantime. Please, review your 
>changes in this part and keep only those which 
>are really needed for some reason (preferably 
>describe the reasons for saving them using 
>comments directly in the code, so that previous 
>mistakes are avoided in the future). 

This is the real problem. In an hypotetical 
chunk of code like 
mov ax,1 {absolutely necessary for video mode} 
mov ax,bx 
the original value must be preserved by a pair 
push/pop because it is overwritten in the logic 
of the flow. For what I could see there's no such 
problem in the graph unit. Instead you are 
telling me the compiler don't handle correctly 
with the construct 
asm 
end['eax'] 
i.e. it left a garbage value, then I point out 
the unit holds only one of these in the 
seg_bytemove, where in all the others assembler 
block they lack, so I manually saved the 
registers on the stack. 

>3) Leak in modes.inc has been addressed by 
>Florian in the meantime. His change isn't 
>completely equal to yours and it isn't clear to 
>me, whether your other changes in this part might 
>be needed too, or not - could you check the 
>latest version whether you still find a need to 
>change something there, please? 

Florian relies on the fact that the pointer of the 
element of the linked list are also be initialised 
to null and avoids to make it twice, I preferred 
in my patch to be more consistent and completely 
clear. However I believe his change works fine. 

>4) Please, try to avoid nothing doing changes 
>like '0' to '00', '2' to '02h', etc. - things 
>like this make it more difficult to review your 
>diffs. I sometimes feel with your changes that 
>you were experimenting in order to find the 
>reason why something was broken until one of your 
>changes finally fixed the issue, but you then 
>kept all the changes, not just the needed one(s). 

You feel good in almost cases, and I imagine that 
you may frighten about a whirl of activity. I only 
try to follow a path from the more suspect issues 
descending to the anothers, and I keep a 
modification whether it makes the source more 
readable for me or someone else. Obviously I 
know that my point of view is not shared by all 
the world. I would also spoke a word on developing 
in general: in my experience we can achieve the best 
result if we accept that bugs don't have a linear 
reduction ever, rather than we fall into stilness 
while finding the perfect way that bring in the 
perfect state. 

>5) It's rather unclear to me why you changed all 
>the various constants to typed ones - this 
>shouldn't be needed normally. See the previous 
>point too. 

Because the constants will be copyied into a sized 
register and/or manipulated with specific bit 
operator. 

>Thanks for your effort to improve FPC and sorry 
>for not responding earlier, I was too busy at 
>that time and Pierre probably too. :-( 

Don't you worry, I am happy to see you again. 
Marco. 


------------------------------------------------------
Passa a Infostrada. ADSL e Telefono senza limiti e senza canone Telecom
http://infostrada.it