[fpc-devel] Re: Graph unit under Go32V2

[Tomas Hajny]

On 14 Mar 07, at 0:23, borsa77 at libero.it wrote:
> On 11 Mar 07, at 21:57, Tomas Hajny wrote: 
 .
 .
> >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.

I'm not sure whether I was clear - there was a 
bug in compiler, but it should be hopefully fixed 
now. If you still find some issues in this area 
(like compiler not saving what should be saved, 
etc.), please, report it in the bug repository 
(unless you can fix it yourself in the compiler 
too, of course).


 .
 .
> >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.

Please, keep only those changes which are 
necessary and revert those, which you tried but 
they didn't contribute to fixing some real issue 
or improving something in the generated code, 
because it gets easier to review your changes and 
you increase the chance of getting the required 
fixes committed to our repository earlier this 
way. I realize that this requires some effort on 
your side due to the way you work, but I believe 
it is important for working with history of 
changes in our repository.


> >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.

That should be normally handled by FPC itself. 
Please, don't add those types unless needed for 
some reason.

Regards

Tomas