Re: [Help-smalltalk] Better compiled method copying

[Holger Hans Peter Freyther]

On Tue, Jun 11, 2013 at 11:30:33PM +0200, Gwenaël Casaccio wrote:

Good Evening,

here some quick comments on the code and commit message.


> When a compiled method is copied some literals (block and closures)
> need to be fixed: they are pointing to the bad method. Also the debug
> information need to be patched to point to the new literals array.

"useless"/"bad". These words carry judgement but there is no poin in
judging. I would very much prefer if you could use a more neutral tone
in your commit messages.

Could you elaborate on how you stumbled across this? When did you copy
the CompiledMethod? What was the usecase?


> +GST_PACKAGE_ENABLE([Tests], [tests])

"Tests" is very generic. What about "SystemTests"?  I understand that
using SUnit is nicer than the GNU autotest framework and personally I
can understand that.


> +    method: aCompiledCode [
> +        <category: 'accessing'>
> +
> +        block method: aCompiledCode
> +    ]


Sounds more like a private method to me, than 'accessing'.

> +    deepCopy [
           ^super deepCopy
> +            fixBlockInformation;
> +            fixDebugInformation: self;
> +            makeLiteralsReadOnly;
               yourself

why didn't this work? Otherwise you will need to adjust your test
case to also test for classes where isPointers evaluates to true.


> +            (literals at: i) class == BlockClosure ifTrue: [
> +                | new_block |
> +                new_block := (literals at: i) deepCopy.
> +                new_block block: new_block block copy.
> +                new_block method: self.
> +                literals at: i put: new_block ]. ]

can you please elaborate on these lines? First youtake a deep copy
and then you take a copy of the deep copied block? Why is that needed?

> +    postCopy [
> +        "Private - Make a deep copy of the descriptor and literals.
> +         Don't need to replace the method header and bytecodes, since they
> +         are integers."
> +
> +        <category: 'private-copying'>
> +
> +        super postCopy.
> +        descriptor := descriptor copy.
> +        literals := literals copy.
> +        self fixBlockInformation.
> +        self makeLiteralsReadOnly.
> +        "literals := literals deepCopy.
> +         self makeLiteralsReadOnly"

time to remove the commented out code as you are doing this now? Did you
do the archology to see if these two lines have ever been enabled in the
last couple of years?


> +    method: aCompiledMethod [
> +     <category: 'accessing'>

it is not really accessing when you modify a class. :)

> +TestCase subclass: TestCompiledMethod [
> +
> +    setUp [
> +        <category: 'setup'>
> +
> +        Object subclass: #Bar.
> +        Object subclass: #Foo.

a tearDown should remove this class too.


> +    testCopy [

...

> +        self assert: old_method ~~ new_method.
> +        self assert: old_method literals ~~ new_method literals.
> +        self assert: old_method getHeader == new_method getHeader.
> +        self assert: old_method descriptor ~~ new_method descriptor.
> +        self assert: old_method descriptor debugInformation ~~ new_method 
> descriptor debugInformation.

matching bytecodes could be added?


> +    testDeepCopy [
> +        <category: 'testing'>

can some code from the above be re-used and also with the below.


> +    ]
> +
> +    testWithNewMethodClass [
> +        <category: 'testing'>


thanks for the patch!