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:
> ------
> [1] https://github.com/hpi-swa-lab/squeak-inbox-talk