It fixes a really terrible memory leak.

Thanks

Andrew

Op maandag 15-12-2008 om 23:52 uur [tijdzone -0500], schreef Andrew Haines:

It fixes a really terrible memory leak.

No.

Not for wait for the testsuite-results (if there are no tests for this particular part, then we even have to wait some longer, as we need users to test it). Then let it settle at least a few days.

Then we'll see.

Joost.

If it is important then I already tested it after Andrew commit and now using it in the fix compiled lhelp application (where it was visible before and was a reason of this fix). No problems found. And memory dont jump too high as before.

IMO you are using very high restrictions for fpc packages. There must be less restrictive rules for fixes that does not touch compiler, rtl and fcl or developers will move their packages outside fpc as it is now with LNet. Who needs this?

Best regards,
Paul Ishenin.

Op dinsdag 16-12-2008 om 17:19 uur [tijdzone +0700], schreef Paul Ishenin:

Yes, that's usefull.

IMO you are using very high restrictions for fpc packages. There must be less restrictive rules for fixes that does not touch compiler, rtl and fcl or developers will move their packages outside fpc as it is now with LNet. Who needs this?

I don't think that we are restrictive at all for packages. In general everything is merged. How's that restrictive?

That lNet isn't included with fpc has more to do with the slow release-process of fpc. This process is slowed down by many reasons, from which one is that people want that we merge fixes after a freeze. Which is what we are talking about now.

How can you expect us to merge a change a few hours after it's commit? We don't even know if the change compiles on all targets, so why should we even consider merging it? We'll always have to wait for the testsuite-run on all tested platforms (which is tonight.)

So in theory we could merge tomorrow. But as we are so close to a release we know that the change won't be tested very well before the release we have to e carefull. So give it a few days. But it also must be tested within fixes, so we can't merge just before we tag 2.2.4.

When this will cause this change to miss the release, we could have a better look.

But how far do you want to go? If all the releases are already build and a new patch comes in? Do all the work over? That way we'll never be able to release a new version again. (for clarity: this is not the case now, I think this patch will make it, but only the test-results can tell)

Joost.

Yes, testsuite is a good indicator. But does it needed in all cases? The main problem in Andrew case was just a typo:

if <condition> then ; do_something - it was just semicolon. Do we need to run testsuite in this case too?

IMO if testsuite showed no negative differences and change was not big then there is no reason to wait that few other days :) In other words someone committer need to look on change and decide: a) it is very trivial change - no reason to wait
b) the change is not trivial - lets wait for few days

Best regards,
Paul Ishenin.

Ok, well whatever is fine with me since I don't really use the fixes branch. But it fixes an enormous memory leak that was caused by

if some condition then ;   // << improperly placed semicolon
  ExtractAFileToMemory;

Searching for tcontrol in lcl.chm used >200 megabytes of memory. And each subsequent search only increased memory usage further.

Regards,

Andrew

Some clear guidelines to know when something can be merged to fixes are useful. Waiting for at least one test suite run is one such guideline that has been proposed. It should not cause any major inconveniences, and at the same time should help with keeping a minimal level of quality for the fixes branch (namely that it builds for all tested platforms, and that the change doesn't cause regressions for tested functionality).

Jonas

And now, can it be merged?

Vincent