On 3/22/07, Quentin Mathé <[EMAIL PROTECTED]> wrote:
Le 22 mars 07 à 00:37, Yen-Ju Chen a écrit :
> Author: yjchen
> Date: Thu Mar 22 00:37:48 2007
> New Revision: 1588
>
> URL: http://svn.gna.org/viewcvs/etoile?rev=1588&view=rev
> Log:
> Fix memory leak for testing instance methods
>
> Modified:
> trunk/Etoile/Frameworks/UnitKit/ChangeLog
> trunk/Etoile/Frameworks/UnitKit/Source/FrameworkSource/UKRunner.m
> trunk/Etoile/Frameworks/UnitKit/Source/TestSource/TestBundle/
> TestThree.m
Hi Yen-Ju,
First thanks for making UnitKit less buggy :-)
However I have questions… :-)
I don't really understand what you try to fix at UKRunner.m level
with this commit. When I wrote this code recently to support class
methods testing, I wanted to do it in the most polymorphic way I can
figure out.
From UKRunner.m latest version:
/* We use local variable so that the testObject will not be
messed up.
What do you mean by messed up?
Because testObject is reused.
For example, two methods are tested, therefore, the loop will run twice.
Then it will run like this:
[testObject ini];
[testObject performSelector: method1];
[testObject release]; <- relased, may be dealloced.
[testObject init]; <- memory leak.
[testObject performSelector: method2];
[testObject release]; <- released again.
And we have to distinguish class and instance
because -init and -release apply to instance.
It's not clear to me why it's a problem since I was testing each time
whether the class object responds to -init or -initForTest (with
[object respondsToSelector: @selector(initForTest) for example)
before calling it… As a side note, the class could responds to -
initForTest if you added +initForTest method to your object
implementation code.
Yes, for -init and initForTest, there is no farm
and the check code can be removed if you want.
At time time, I was more conservative about memory management.
I could be wrong but my thinking from what I know of objc runtime is
the following one… If you call -respondsToSelector: on an instance,
it checks whether the class responds to this message because instance
methods are stored at class level. Now if you call -
respondsToSelector: on a class, it checks whether the metaclass
reponds to this message because class methods are stored at metaclass
level.
And -release also dealloc object, which will cause memory
problem */
This could be a problem though, if class object reponds to selector
'release' (I didn't test it). More on this at the end.
See above. class object is fine, but instance object will be
released multiple times.
/* FIXME: there is a memory leak because testObject comes
here as allocated to tell whether it is class or instance.
We can dealloc it here, but it is not really a good practice.
Object is better to be pass as autoreleased. */
I don't see the memory leak.
It is because testObject was -alloc before it comes to the method.
We didn't release it here, and it is never released in the end.
That is the leak I am taking about.
Here is my analysis…
in -runTestsInClass:
/* Test class methods */
testMethods = UKTestMethodNamesFromClass(object_get_meta_class
(testClass));
[self runTests:testMethods onObject:testClass];
/* Test instance methods */
testMethods = UKTestMethodNamesFromClass(testClass);
[self runTests:testMethods onObject: [testClass alloc]]; <--
here is the allocation
in -runTests:onObject:
if ([testObject respondsToSelector: @selector
(releaseForTest)])
{
[testObject releaseForTest]; <-- here is the
deallocation
(possibility 1)
}
else if ([testObject respondsToSelector:
@selector(release)])
{
[testObject release]; <-- here is the
deallocation (possibility 2)
}
Now if its a class object, we don't call -alloc on it, therefore we
just need to check -release isn't call on it or that -release doesn't
do anything on a class object.
Your analyze is right for the first loop.
But if loop run more than once, the same testObject will be
dealloced more than once.
That is why I use a local variable and alloc local instance for the loop
while keep testObject untouched.
To sum up:
1. -init and -initForTest can apply to instance and class. The code
is too conservative.
(PS. class method is like function call. I don't see the use of
+init or +initForTest.
It should be done in +instanize, I guess.).
2. Better not to reused testObject because it will be released
multipl times in the loop.
Yen-Ju
Cheers,
Quentin.
--
Quentin Mathé
[EMAIL PROTECTED]
_______________________________________________
Etoile-dev mailing list
[email protected]
https://mail.gna.org/listinfo/etoile-dev
