(Forking this thread due to difference of concerns ... :-))
I'm also not sure whether these methods belong to ProtoObject or not.
This is an interesting problem, see also the recent thread about cleaning up ProtoObject. I agree with Eliot that a good ProtoObject is as small and stupid as possible in order to be really useful for writing powerful proxies or decorators. It should be possible to wrap nil into an ObjectTracer, for example! To put the issue in a nutshell, try out the following:
newNil := UndefinedObject basicNew.
nilTracer := ObjectTracer on: newNil.
nilTracer isNil. "false"
nilTracer perform: #isNil. "true"
Here's a similar issue:
specialClass := Object newSubclass. specialClass compile: 'class ^42'. specialObj := specialClass new. specialObj class. "Object1" specialObj perform: #class. "42"
(By the way, we are not even consistent with ourself in this concern:
([specialObj class] newProcess runUntil: #willReturn) suspendedContext top. "42"
) Pooh, that's both really not nice ... Basically, inlining of #isNil & Co. is an optimization and IMHO it should not affect the basic concept of how messages are dispatched. Too less inlining affects performance, but too much inlining introduces conceptional errors. Would it be a big performance impact if we made inlining optional? Here is a short proposal:
We could tell every bytecode interpreter to check for any receiver if it agrees on being deprived of messages that can be inlined. We could decide this per receiver class and cache these answers in the VM. Hypothetical example:
Object >> #acceptsInlining
"This message is never sent, but it's existance signalizes the VM that it is okay to inline popular messages such as #isNil or #ifTrue:."
If this concept is too expensive or too confusing, we could also add an instance variable to the class object. Anyway, something like this. That way we could make the concept of inlining optional. If an interpreter evaluated {nilTracer isNil}, it would have to do the following (I'm following the simulation code in #interpretNextSistaV1InstructionFor:. Disclaimer: Absolutely free of optimizations!)
... "short sends" (div16 = 6 and: [rcvr respondsTo: #acceptsInlining]) ifTrue: [^client send: (Smalltalk specialSelectorAt: offset + 1) super: false numArgs: (Smalltalk specialNargsAt: offset + 1)]. ... "otherwise, message will be dispatched regularly"
Some similar checks would be required in case of primitives that are generated for inlined methods, such as 257f.:
... ((primIndex := meth primitive) > 0 and: [ (self isInliningPrimitive: primitive) not or: [rcvr respondsTo: #acceptsInlining]]) ifTrue: [val := self doPrimitive: primIndex method: meth receiver: rcvr args: arguments. (self isPrimFailToken: val) ifFalse: [^val]] ... "otherwise, message will be dispatched regularly"
How would you think of this concept in general? I'm excited to hear your feedback! :-)
Best, Christoph
________________________________ Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Levente Uzonyi leves@caesar.elte.hu Gesendet: Samstag, 21. März 2020 20:25 Uhr An: The general-purpose Squeak developers list Betreff: Re: [squeak-dev] #isNilOr:, #notNilAnd:
Hi Christoph,
They are okay for scripts, but we know that methods like that always end up being used out of scripts, and are even misused every now and then. They are especially susceptible to misuse when their return value is ignored; in which case they are both equivalent to #ifNotNil:.
I'm also not sure whether these methods belong to ProtoObject or not. I know the other nil handling methods are there as well, but that doesn't mean all such methods belong there. E.g. #ifNil:/#ifNotNil: should be there because the current compiler and VM will make those available for all objects provided the methods are inlined. But #isNil and #notNil have no such properties, so they belong more to Object than ProtoObject in my opinion.
Levente
On Mon, 16 Mar 2020, Thiede, Christoph wrote:
Hi all,
after the release has been managed successfully, I would like to kindly push this proposal. In the latest months, I found myself to really often desiring #isNilOr: or #notNilAnd:. How would you think about these convenience selectors? :-)
Best,
Christoph
Von: Thiede, Christoph Gesendet: Mittwoch, 11. Dezember 2019 11:01:44 An: Squeak Dev Betreff: #isNilOr:, #notNilAnd:
Hi all, just another idea for a hypothetic extension to ProtoObject:
ProtoObject >> #isNilOr: aBlock
^ aBlock cull: self
UndefinedObject >> #isNilOr: aBlock
^ true
ProtoObject >> #notNilAnd: aBlock
^ aBlock cull: self
UndefinedObject >> #notNilAnd: aBlock
^ false
Examples:
parentObject notNilAnd: #canBrowseSubtopic.
helpClass notNilAnd: [:hC| hC usesCodeStyling].
self selectionIndex isNilOr: [:index | index > 2].
anObject isNilOr: #isZero.
foo isNilOr: #isNotEmpty.
Just a bit syntactic sugar for idiomatic problems such as below. In particular, I found several possible applications for #notNilAnd: in my image.
Go ahead and tell me why it would be a bad idea :)
Best,
Christoph
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Thiede, Christoph Gesendet: Freitag, 25. Oktober 2019 19:21 Uhr An: gettimothy via Squeak-dev Betreff: Re: [squeak-dev] The Inbox: HelpSystem-Core-ct.123.mcz
If you're needing a decision, I'd vote for option 3.
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Mittwoch, 16. Oktober 2019 11:14:15 An: gettimothy via Squeak-dev Betreff: Re: [squeak-dev] The Inbox: HelpSystem-Core-ct.123.mcz Quick suggestion on the formatting. This one: ^ self canBrowseTopic or: [ parentTopic ifNotNil: #canBrowseSubtopic ifNil: [false]]
could become:
^ self canBrowseTopic or: [ parentTopic ifNil: [false] ifNotNil: [:topic | topic canBrowseSubtopic]]
or:
^ self canBrowseTopic or: [parentTopic notNil and: [parentTopic canBrowseSubtopic]]
Hmmm... what are other opinions on this? There is no need for #ifNil/ifNotNil in such a boolean expression?
Best, Marcel
Am 13.10.2019 21:04:19 schrieb commits@source.squeak.org <commits@source.squeak.org>: A new version of HelpSystem-Core was added to project The Inbox: http://source.squeak.org/inbox/HelpSystem-Core-ct.123.mcz ==================== Summary ==================== Name: HelpSystem-Core-ct.123 Author: ct Time: 13 October 2019, 9:04:08.373932 pm UUID: dec7ceca-320f-d945-8d2a-c2f6a5e49a52 Ancestors: HelpSystem-Core-ct.120 Refactors HelpBrowser menu: Move menu stuff from HelpBrowser into HelpTopic hierarchy in favor of a better object design Thanks again, Marcel :-) =============== Diff against HelpSystem-Core-ct.120 =============== Item was added: + ----- Method: AbstractHelpTopic>>browseTopicFromParent: (in category 'tools') ----- + browseTopicFromParent: parentTopic + + self canBrowseTopic + ifTrue: [^ self browseTopic]. + parentTopic canBrowseSubtopic + ifTrue: [^ parentTopic browseSubtopic: self]. + ! Item was added: + ----- Method: AbstractHelpTopic>>canBrowseSubtopic (in category 'testing') ----- + canBrowseSubtopic + + ^ false! Item was added: + ----- Method: AbstractHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ false! Item was added: + ----- Method: AbstractHelpTopic>>canBrowseTopicFromParent: (in category 'testing') ----- + canBrowseTopicFromParent: parentTopic + + ^ self canBrowseTopic or: [ + parentTopic ifNotNil: #canBrowseSubtopic ifNil: [false]]! Item was added: + ----- Method: AbstractHelpTopic>>topicMenu:parentTopic: (in category 'menus') ----- + topicMenu: aMenu parentTopic: parentTopic + + aMenu + add: 'Inspect (i)' translated target: self action: #inspect; + add: 'Explore (I)' translated target: self action: #explore. + (self canBrowseTopicFromParent: parentTopic) + ifTrue: [ + aMenu add: 'Browse (b)' translated + target: self + selector: #browseTopicFromParent: + argumentList: {parentTopic} ]. + + ^ aMenu! Item was added: + ----- Method: AbstractHelpTopic>>topicMenuKey:fromParent: (in category 'menus') ----- + topicMenuKey: aChar fromParent: parentTopic + + aChar + caseOf: { + [$b] -> [(self canBrowseTopicFromParent: parentTopic) + ifTrue: [ self browseTopicFromParent: parentTopic ]]. + [$i] -> [self inspect]. + [$I] -> [self explore] } + otherwise: [^ false]. + ^ true! Item was added: + ----- Method: ClassAPIHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ true! Item was added: + ----- Method: ClassBasedHelpTopic>>canBrowseSubtopic (in category 'testing') ----- + canBrowseSubtopic + + ^ true! Item was added: + ----- Method: ClassBasedHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ true! Item was added: + ----- Method: DirectoryBasedHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ true! Item was added: + ----- Method: FileBasedHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ true! Item was removed: - ----- Method: HelpBrowser>>browseTopic (in category 'actions') ----- - browseTopic - - ^ (self currentTopic respondsTo: #browseTopic) - ifTrue: [self currentTopic browseTopic] - ifFalse: [self currentParentTopic browseSubtopic: self currentTopic]! Item was removed: - ----- Method: HelpBrowser>>canBrowseTopic (in category 'testing') ----- - canBrowseTopic - - ^ (self currentTopic respondsTo: #browseTopic) - or: [self currentParentTopic respondsTo: #browseSubtopic:]! Item was removed: - ----- Method: HelpBrowser>>exploreTopic (in category 'actions') ----- - exploreTopic - - ^ self currentTopic explore! Item was removed: - ----- Method: HelpBrowser>>inspectTopic (in category 'actions') ----- - inspectTopic - - ^ self currentTopic inspect! Item was changed: ----- Method: HelpBrowser>>treeKey:from:event: (in category 'menus') ----- treeKey: aChar from: aView event: anEvent anEvent anyModifierKeyPressed ifFalse: [^ false]. + ^ (self currentTopic topicMenuKey: aChar fromParent: self currentParentTopic)! - aChar - caseOf: { - [$b] -> [self browseTopic]. - [$i] -> [self inspectTopic]. - [$I] -> [self exploreTopic]. } - otherwise: [^ false]. - ^ true! Item was changed: ----- Method: HelpBrowser>>treeListMenu: (in category 'menus') ----- treeListMenu: aMenu + ^ self currentTopic + ifNil: [aMenu] + ifNotNil: [:topic | topic + topicMenu: aMenu + parentTopic: self currentParentTopic]! - self currentTopic ifNil: [^ aMenu]. - - aMenu - add: 'Inspect (i)' action: #inspectTopic; - add: 'Explore (I)' action: #exploreTopic. - - self canBrowseTopic ifTrue: [ - aMenu - addLine; - add: 'Browse (b)' action: #browseTopic]. - - ^ aMenu! Item was added: + ----- Method: MethodListHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ true! Item was added: + ----- Method: PackageAPIHelpTopic>>canBrowseTopic (in category 'testing') ----- + canBrowseTopic + + ^ true!