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