Re: [Help-smalltalk] Better compiled method copying

[Paolo Bonzini]

Il 23/06/2013 19:45, Holger Hans Peter Freyther ha scritto:
> 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.

Or KernelTests.  It's a pity that Kernel is not a regular package. :(

> 
>> +    method: aCompiledCode [
>> +        <category: 'accessing'>
>> +
>> +        block method: aCompiledCode
>> +    ]
> 
> 
> Sounds more like a private method to me, than 'accessing'.

Agreed.

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

No underscores in variable names.

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

I think it's better to create the classes unconditionally.
setUp/tearDown can create and remove the methods, though.

Paolo

> 
>> +    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!
> 
> 
> _______________________________________________
> help-smalltalk mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/help-smalltalk
>