David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.1442.mcz
==================== Summary ====================
Name: System-dtl.1442 Author: dtl Time: 4 January 2024, 8:07:18.703979 pm UUID: 2ef3f6e2-a4fd-4d82-a79c-6ed6f4eb05b2 Ancestors: System-ct.1441
Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM
=============== Diff against System-ct.1441 ===============
Item was changed: ----- Method: SmalltalkImage>>vmVMMakerVersion (in category 'system attributes') ----- vmVMMakerVersion + "Answer the version number of the VMMaker package from which the main VM was compiled, + or zero if the version number cannot be determined." - "Answer the version number of the VMMaker package from which the main VM was compiled." "Smalltalk vmVMMakerVersion"
| vmMakerID | + vmMakerID := self vmVersion substrings + detect: [:token| token beginsWith: 'VMMaker'] + ifNone: [ '0' ]. - vmMakerID := self vmVersion substrings detect: [:token| token beginsWith: 'VMMaker']. ^Integer readFrom: (vmMakerID subStrings: '.') last!
Hi Dave,
why not answer nil instead of 0? I think with your change, non-VMMaker Spur VMs will start to skip all ImageSegmentTests. Maybe senders of #vmVMMakerVersion should also check it for nil or send a new check #vmIsVMMaker before?
Best, CHristoph
--- Sent from Squeak Inbox Talk
On 2024-01-05T02:08:55+00:00, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.1442.mcz
==================== Summary ====================
Name: System-dtl.1442 Author: dtl Time: 4 January 2024, 8:07:18.703979 pm UUID: 2ef3f6e2-a4fd-4d82-a79c-6ed6f4eb05b2 Ancestors: System-ct.1441
Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM
=============== Diff against System-ct.1441 ===============
Item was changed: ----- Method: SmalltalkImage>>vmVMMakerVersion (in category 'system attributes') ----- vmVMMakerVersion
- "Answer the version number of the VMMaker package from which the main VM was compiled,
- or zero if the version number cannot be determined."
- "Answer the version number of the VMMaker package from which the main VM was compiled."
"Smalltalk vmVMMakerVersion"
| vmMakerID |
- vmMakerID := self vmVersion substrings
- detect: [:token| token beginsWith: 'VMMaker']
- ifNone: [ '0' ].
- vmMakerID := self vmVersion substrings detect: [:token| token beginsWith: 'VMMaker'].
^Integer readFrom: (vmMakerID subStrings: '.') last!
It is because vmVMMakerVersion is expected to answer a number, and nil is not a number.
It's similar to what you might do for a method that is expected to answer a collection. In that case, you might choose to answer #() as a default, but it would be best not to answer nil because all senders would then need to do nil checks. By choosing #() as a default, the normal behavior will be error free and any sender that cares can check for #isEmpty.
This is not just a Smalltalk thing, the Java folks might have learned this the hard way but they have come to the same conclusion:
https://medium.com/javarevisited/just-dont-return-null-dcdf5d77128f
Dave
On 2024-01-06 19:41, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Dave,
why not answer nil instead of 0? I think with your change, non-VMMaker Spur VMs will start to skip all ImageSegmentTests. Maybe senders of #vmVMMakerVersion should also check it for nil or send a new check #vmIsVMMaker before?
Best, CHristoph
_Sent from __Squeak Inbox Talk [1]_
On 2024-01-05T02:08:55+00:00, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.1442.mcz
==================== Summary ====================
Name: System-dtl.1442 Author: dtl Time: 4 January 2024, 8:07:18.703979 pm UUID: 2ef3f6e2-a4fd-4d82-a79c-6ed6f4eb05b2 Ancestors: System-ct.1441
Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM
=============== Diff against System-ct.1441 ===============
Item was changed: ----- Method: SmalltalkImage>>vmVMMakerVersion (in category 'system attributes') ----- vmVMMakerVersion
"Answer the version number of the VMMaker package from which the
main VM was compiled,
or zero if the version number cannot be determined."
"Answer the version number of the VMMaker package from which the
main VM was compiled." "Smalltalk vmVMMakerVersion"
| vmMakerID |
vmMakerID := self vmVersion substrings
detect: [:token| token beginsWith: 'VMMaker']
ifNone: [ '0' ].
vmMakerID := self vmVersion substrings detect: [:token| token
beginsWith: 'VMMaker']. ^Integer readFrom: (vmMakerID subStrings: '.') last!
Links: ------ [1] https://github.com/hpi-swa-lab/squeak-inbox-talk
Hi Dave,
I would argue that answering a "somewhat plausible" default value such as zero does often do more harm than answering a clear null object because they unncessarily prolong the fault propagation chain, making it harder to debug errors. As mentioned before, your change causes to skip all ImageSegmentTests in non-VMMaker VMs. A two-valued logic like in Smalltalk vmVMMakerVersion < 2295 cannot accurately cover the absence of VMMaker. I think ImageSegmentTest class>>#testSelectors (and any other senders of #vmVMMakerVersion, analogously) should look like this instead:
testSelectors
(Smalltalk isRunningSpur and: [Smalltalk vmIsVMMaker] "<-- added!" and: [Smalltalk vmVMMakerVersion < 2295]) ifTrue: "The ImageSegment Test is known to not work on older Spur OSVMs with the prospect of crashing. #expectedFailures does not cut it here; don't even try to run them" [^#()]. ^super testSelectors
The article you shared was an interesting read but I think it mostly applies to typed languages like Java which handle nullability in different ways. Returning an Optional.empty() instead of null just makes sense because the compiler will warn you when you assume the former to be available but not when you assume the latter to be available (and btw, without being proficient in Java, many compilers seem to support @NotNull annotations today). Squeak does not have tools like these (because we believe in the simplicity of dynamic typing and exploratory programming instead), but perhaps the most idiomatic way to describe both cases would be a #vmWithVMMakerVersionDo:ifNotVMMaker: message. This way, senders *cannot* ignore one of these cases. Just my $0.02. :-)
Best, Christoph
--- Sent from Squeak Inbox Talk
On 2024-01-06T21:16:20+00:00, lewis@mail.msen.com wrote:
It is because vmVMMakerVersion is expected to answer a number, and nil is not a number.
It's similar to what you might do for a method that is expected to answer a collection. In that case, you might choose to answer #() as a default, but it would be best not to answer nil because all senders would then need to do nil checks. By choosing #() as a default, the normal behavior will be error free and any sender that cares can check for #isEmpty.
This is not just a Smalltalk thing, the Java folks might have learned this the hard way but they have come to the same conclusion:
https://medium.com/javarevisited/just-dont-return-null-dcdf5d77128f
Dave
On 2024-01-06 19:41, christoph.thiede(a)student.hpi.uni-potsdam.de wrote:
Hi Dave,
why not answer nil instead of 0? I think with your change, non-VMMaker Spur VMs will start to skip all ImageSegmentTests. Maybe senders of #vmVMMakerVersion should also check it for nil or send a new check #vmIsVMMaker before?
Best, CHristoph
_Sent from __Squeak Inbox Talk [1]_
On 2024-01-05T02:08:55+00:00, commits(a)source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.1442.mcz
==================== Summary ====================
Name: System-dtl.1442 Author: dtl Time: 4 January 2024, 8:07:18.703979 pm UUID: 2ef3f6e2-a4fd-4d82-a79c-6ed6f4eb05b2 Ancestors: System-ct.1441
Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM
=============== Diff against System-ct.1441 ===============
Item was changed: ----- Method: SmalltalkImage>>vmVMMakerVersion (in category 'system attributes') ----- vmVMMakerVersion
- "Answer the version number of the VMMaker package from which the
main VM was compiled,
- or zero if the version number cannot be determined."
- "Answer the version number of the VMMaker package from which the
main VM was compiled." "Smalltalk vmVMMakerVersion"
| vmMakerID |
- vmMakerID := self vmVersion substrings
- detect: [:token| token beginsWith: 'VMMaker']
- ifNone: [ '0' ].
- vmMakerID := self vmVersion substrings detect: [:token| token
beginsWith: 'VMMaker']. ^Integer readFrom: (vmMakerID subStrings: '.') last!
Links:
On 2024-01-13 19:24, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Dave,
I would argue that answering a "somewhat plausible" default value such as zero does often do more harm than answering a clear null object because they unncessarily prolong the fault propagation chain, making it harder to debug errors. As mentioned before, your change causes to skip all ImageSegmentTests in non-VMMaker VMs. A two-valued logic like in Smalltalk vmVMMakerVersion < 2295 cannot accurately cover the absence of VMMaker. I think ImageSegmentTest class>>#testSelectors (and any other senders of #vmVMMakerVersion, analogously) should look like this instead:
testSelectors
(Smalltalk isRunningSpur and: [Smalltalk vmIsVMMaker] _"<-- added!"_ and: [Smalltalk vmVMMakerVersion < 2295]) ifTrue: "The ImageSegment Test is known to not work on older Spur OSVMs with the prospect of crashing. #expectedFailures does not cut it here; don't even try to run them" [^#()]. ^super testSelectors
Hi Christoph,
The purpose of my update is in the commit comment:
"Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM"
This was blocking the update stream for V3 images. It is not an issue when running on OSVM, but on an interpreter VM it caused a 'NotFound: Object is not in the collection.' error because the vmVMMakerVersion method is trying to parse out the substrings in system attribute 1004, which might or might not contain the string 'VMMaker'.
Usually this would be annoying but harmless, but when it happens in a package postscript, it blocks the update stream, and that is why I let vmVMMakerVersion default to 0 rather than fail outright.
This is really not very important, and if you want to revert it don't mind. But it seemed to me to be a good thing to clean up.
-1 for #vmIsVMMaker. VMMaker is the package name that is used for OSVM and also for the traditional VM, and maybe some others as well. So if you needed a test for "is opensmalltalk-vm" then maybe you would call it #isOSVM. But I don't see any real need for this. Or maybe I am misunderstanding what you mean by "non-VMMaker VMs".
Dave
On 2024-01-13 23:43, lewis@mail.msen.com wrote:
On 2024-01-13 19:24, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Dave,
I would argue that answering a "somewhat plausible" default value such as zero does often do more harm than answering a clear null object because they unncessarily prolong the fault propagation chain, making it harder to debug errors. As mentioned before, your change causes to skip all ImageSegmentTests in non-VMMaker VMs. A two-valued logic like in Smalltalk vmVMMakerVersion < 2295 cannot accurately cover the absence of VMMaker. I think ImageSegmentTest class>>#testSelectors (and any other senders of #vmVMMakerVersion, analogously) should look like this instead:
testSelectors
(Smalltalk isRunningSpur and: [Smalltalk vmIsVMMaker] _"<-- added!"_ and: [Smalltalk vmVMMakerVersion < 2295]) ifTrue: "The ImageSegment Test is known to not work on older Spur OSVMs with the prospect of crashing. #expectedFailures does not cut it here; don't even try to run them" [^#()]. ^super testSelectors
Hi Christoph,
The purpose of my update is in the commit comment:
"Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM"
This was blocking the update stream for V3 images. It is not an issue when running on OSVM, but on an interpreter VM it caused a 'NotFound: Object is not in the collection.' error because the vmVMMakerVersion method is trying to parse out the substrings in system attribute 1004, which might or might not contain the string 'VMMaker'.
Usually this would be annoying but harmless, but when it happens in a package postscript, it blocks the update stream, and that is why I let vmVMMakerVersion default to 0 rather than fail outright.
This is really not very important, and if you want to revert it don't mind. But it seemed to me to be a good thing to clean up.
-1 for #vmIsVMMaker. VMMaker is the package name that is used for OSVM and also for the traditional VM, and maybe some others as well. So if you needed a test for "is opensmalltalk-vm" then maybe you would call it #isOSVM. But I don't see any real need for this. Or maybe I am misunderstanding what you mean by "non-VMMaker VMs".
Dave
Sorry for replying to myself but I can't resist adding some historical background here.
The original "back to the future" Squeak VM was part of the Squeak image, so the VM version could easily be identified simply from the version of Squeak from which it was built.
The VM version was made available as system attribute 1004, so checking system attribute 1004 might give you a string that looks like this:
Smalltalk getSystemAttribute: 1004 ==> 'Squeak4.6 of 21 July 2021 [latest update: #15118]'
Over time, the VM was moved out of the image and maintained separately in VMMaker and related packages. As this happened, the version of the image itself became mostly irrelevant for identifying the VM version. In OSVM (VMMaker.oscog) the system attribute was changed to answer something more useful:
Smalltalk getSystemAttribute: 1004 'Open Smalltalk Cog[Spur] VM [CoInterpreterPrimitives VMMaker.oscog-eem.3339]'
Other VMs may do other things. Notably, SqueakJS looks like this:
Smalltalk getSystemAttribute: 1004 ==> 'SqueakJS 1.0.4 [VMMakerJS-bf.17 VMMaker-bf.353]'
Getting back to the vmVMMakerVersion topic, we now have these variations:
OSVM: Smalltalk vmVMMakerVersion ==> 3339 classic VM: Smalltalk vmVMMakerVersion ==> 0 SqueakJS: Smalltalk vmVMMakerVersion ==> 353
This may or may not do the right thing for SqueakJS (which IMHO is very important), so I have to concede Christoph's point - we may need some additional check for 'is running OSVM'.
Dave
Hi Christoph --
answering a clear null object
No, "nil" is not a null object. In this case, 0 would be the null object because it is part of the domain. An extra #vmIsWMMaker also contradicts with the idea of a transparent null object.
I agree with Dave here.
Best, Marcel
Am 13.01.2024 20:25:08 schrieb Thiede, Christoph christoph.thiede@student.hpi.uni-potsdam.de:
Hi Dave,
I would argue that answering a "somewhat plausible" default value such as zero does often do more harm than answering a clear null object because they unncessarily prolong the fault propagation chain, making it harder to debug errors. As mentioned before, your change causes to skip all ImageSegmentTests in non-VMMaker VMs. A two-valued logic like in Smalltalk vmVMMakerVersion < 2295 cannot accurately cover the absence of VMMaker. I think ImageSegmentTest class>>#testSelectors (and any other senders of #vmVMMakerVersion, analogously) should look like this instead:
testSelectors
(Smalltalk isRunningSpur and: [Smalltalk vmIsVMMaker] "<-- added!" and: [Smalltalk vmVMMakerVersion < 2295]) ifTrue: "The ImageSegment Test is known to not work on older Spur OSVMs with the prospect of crashing. #expectedFailures does not cut it here; don't even try to run them" [^#()]. ^super testSelectors
The article you shared was an interesting read but I think it mostly applies to typed languages like Java which handle nullability in different ways. Returning an Optional.empty() instead of null just makes sense because the compiler will warn you when you assume the former to be available but not when you assume the latter to be available (and btw, without being proficient in Java, many compilers seem to support @NotNull annotations today). Squeak does not have tools like these (because we believe in the simplicity of dynamic typing and exploratory programming instead), but perhaps the most idiomatic way to describe both cases would be a #vmWithVMMakerVersionDo:ifNotVMMaker: message. This way, senders *cannot* ignore one of these cases. Just my $0.02. :-)
Best, Christoph
--- Sent from Squeak Inbox Talkhttps://github.com/hpi-swa-lab/squeak-inbox-talk
On 2024-01-06T21:16:20+00:00, lewis@mail.msen.com wrote:
It is because vmVMMakerVersion is expected to answer a number, and nil is not a number.
It's similar to what you might do for a method that is expected to answer a collection. In that case, you might choose to answer #() as a default, but it would be best not to answer nil because all senders would then need to do nil checks. By choosing #() as a default, the normal behavior will be error free and any sender that cares can check for #isEmpty.
This is not just a Smalltalk thing, the Java folks might have learned this the hard way but they have come to the same conclusion:
https://medium.com/javarevisited/just-dont-return-null-dcdf5d77128f
Dave
On 2024-01-06 19:41, christoph.thiede(a)student.hpi.uni-potsdam.de wrote:
Hi Dave,
why not answer nil instead of 0? I think with your change, non-VMMaker Spur VMs will start to skip all ImageSegmentTests. Maybe senders of #vmVMMakerVersion should also check it for nil or send a new check #vmIsVMMaker before?
Best, CHristoph
_Sent from __Squeak Inbox Talk [1]_
On 2024-01-05T02:08:55+00:00, commits(a)source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.1442.mcz
==================== Summary ====================
Name: System-dtl.1442 Author: dtl Time: 4 January 2024, 8:07:18.703979 pm UUID: 2ef3f6e2-a4fd-4d82-a79c-6ed6f4eb05b2 Ancestors: System-ct.1441
Do not fail loading package postscript when calling upscaleDisplayOnHighDPI: if VM is not OSVM
=============== Diff against System-ct.1441 ===============
Item was changed: ----- Method: SmalltalkImage>>vmVMMakerVersion (in category 'system attributes') ----- vmVMMakerVersion
- "Answer the version number of the VMMaker package from which the
main VM was compiled,
- or zero if the version number cannot be determined."
- "Answer the version number of the VMMaker package from which the
main VM was compiled." "Smalltalk vmVMMakerVersion"
| vmMakerID |
- vmMakerID := self vmVersion substrings
- detect: [:token| token beginsWith: 'VMMaker']
- ifNone: [ '0' ].
- vmMakerID := self vmVersion substrings detect: [:token| token
beginsWith: 'VMMaker']. ^Integer readFrom: (vmMakerID subStrings: '.') last!
Links:
squeak-dev@lists.squeakfoundation.org