[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Help-smalltalk] Better compiled method copying
From: |
Paolo Bonzini |
Subject: |
Re: [Help-smalltalk] Better compiled method copying |
Date: |
Thu, 27 Jun 2013 10:21:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
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
>