[fpc-devel] Graph unit under Go32V2

[Tomas Hajny]

On 10 Mar 07, at 19:47, borsa77 at libero.it wrote:

Hi Marco,

> Reminder. 


Good that you sent this reminder. ;-) 
Unfortunately, your diff cannot be used without 
changes and I don't have enough time to dig into 
it deeply. The following points should be 
addressed before applying it:

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.

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

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?

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

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.

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

Tomas