Follow-up from "Don't crash if a callback doesn't return an expected array of values"
The following discussion from !405 (merged) should be addressed:
-
@ptomato started a discussion: (+2 comments) Well, I figured out why this is. The result of
JS::ToNumber()
onundefined
is NaN. The return type of this vfunc isglong
whereas the other vfuncs that return NaN are allgfloat
, so that's why we get NaNs returned from all the other functions but not here. I suspect (from testing on godbolt) that the compiler is converting NaN to 64-bit int withcvttsd2si
on x86-64, and that's where the value of –9223372036854775808 (0x8000000000000000) is coming from.Since this vfunc returns a glong, there's no way we can ever get a NaN from it. So I think it's sufficient to change this expectation to
.toBeDefined()
or something like that.This was pre-existing, however, with this merge request we are now introducing an inconsistency: we get 0x8000000000000000 if we return undefined as a lone integer return value, and 0 if we return undefined as one of multiple integer return values, as in
vfunc_return_value_and_multiple_out_parameters
. I think it's probably worth fixing this. Alternatively, I could be convinced that not returning the correct type from your vfunc is undefined behaviour as far as GJS is concerned, in that case we can just un-pend the test and expect the log message about the value being outside the range of a JS Number. -
Ok, I've changed it to use
.toBeDefined()
, however you're right that this MR exposes an inconsistency (more than introducing it I think), given that this was already the behavior.So, I'm not sure how to fix that though, so given the MR was to focus on non-crashing mostly, maybe we can leave the two "simple
return undefined
" cases aspend
in case we know how to make them behave the same way, and address it somewhere else.