Reminder.

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

On 10 Mar 07, at 19:47, 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

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.

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.

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.

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

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

.
.

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

.
.

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.

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

Regards

Tomas

I am still refer to the 2.0.4 version, before the revision 5696 where the used registers in the assembler blocks are almost not saved. If it needs theirs original value must be preserved disregarding the bug of the compiler (as I think) then I will make a comparison with the last version to find such remaining issues. Otherwise, considering the
modification applyied by Florian Klaempfl and what you claim unnecessary, the work on this patch may be closed.

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,

Yes, I agree.

------------------------------------------------------ Leggi GRATIS le tue mail con il telefonino i-mode™ di Wind http://i-mode.wind.it