On Sun, Feb 1, 2015 at 7:39 PM, Nicolai Hess <[email protected]> wrote:
> > > 2015-02-01 10:52 GMT+01:00 Ben Coman <[email protected]>: > >> >> Looking into Image locking problems [1] caused by a recursive array such >> as this... >> >> literalArray := #( 1 2 3 ). >> literalArray at: 3 put: literalArray. >> >> I find that "literalArray printString" locks the image due to >> Array>>printOn: use of the recursive #shouldBePrintedAsLiteral method. Now >> its implementation is identical to #isLiteral and indeed "literalArray >> isLiteral" also locks the Image. So comparing implementors of #isLiteral... >> >> >> > > Squeak uses a Set to store all visited elements for > shouldBePrintedAsLiteral and this protects against the recursive loop. > > shouldBePrintedAsLiteralVisiting: aSet > self class == Array ifFalse: > [^false]. > (aSet includes: self) ifTrue: > [^false]. > aSet add: self. > ^self allSatisfy: [:each | each shouldBePrintedAsLiteralVisiting: aSet] > > > isn't there a common pattern to handle this kind of potential endless > recursion? > There was a squeak discussion including a passing suggestion for a generic DepthFirstfTraversal safe against cyclic structures... http://forum.world.st/The-Trunk-Collections-ul-564-mcz-td4741229.html > Object>>isLiteral ^false >> Boolean>>isLiteral ^true >> Character>>isLiteral ^true >> Integer>>isLiteral ^true >> String>>isLiteral ^true >> UndefinedObject>>isLiteral ^true >> >> ByteArray>>isLiteral ^self class == ByteArray >> Float>>isLiteral ^self isFinite "^(self - self) = 0.0" >> ScaledDecimal>>isLiteral ^denominator = 1 or: [(10 raisedTo: >> scale)\\denominator = 0] >> >> Array>>isLiteral ^self class == Array and: [self allSatisfy: [:each | >> each isLiteral]] >> >> ...I find most are very basic (might even say deterministic), with the >> recursion of Array>>isLiteral seeming an annomaly. Also, the big IF >> condition in Array>>printOn: smells like a design decision being made at >> runtime (Valloud AMCOS p12). >> >> Array>>printOn: aStream >> self shouldBePrintedAsLiteral ifTrue: [self printAsLiteralFormOn: >> aStream. ^ self]. >> self isSelfEvaluating ifTrue: [self printAsSelfEvaluatingFormOn: aStream. >> ^ self]. >> super printOn: aStream >> >> Flipping between two printString formats seems like selecting between two >> class types. Indeed, if we had a LiteralArray class, there would be no need >> for its printOn: to recursively search to determine its form, thus allowing >> #printStringLimitedTo: to do its thing to protect against infinite >> recursion. >> >> Also, instead of a recursive Array>>isLiteral we'd have something like >> LiteralArray>>isLiteral ^true >> Array>>isLiteral ^false >> which seems to align much better with the pattern of the other #isLiteral >> implementors. >> >> I notice there is both RBArrayNode and RBLiteralArrayNode. >> >> So what are the wider concerns that might apply? >> (In particular, I'm not sure how the #isSelfEvaluating (which is also >> recursive) fits into the big picture) >> >> cheers -ben >> >> [1] https://www.mail-archive.com/[email protected]/msg25156.html >> > > @Stef/Marcus, As part of this, since Array>>#shouldBePrintedAsLiteral implementation is the same as the Array>>isLiteral I was going to suggest removing the former, but I see the introduction of #shouldBePrintedAsLiteral was popular with you guys [1]. Is there some reason why Array>>shouldBePrintedAsLiteral should not be "^self isLiteral" or just removed, so Object>>shouldBePrintedAsLiteral is inherited ? [1] http://pharo-dev.pharo.narkive.com/KVAEE1Xv/pharo-project-issue-3426-in-pharo-shouldbeprintedasliteral-instead-of-isliteral cheers -ben
