fix toShortSafely: yes we can safely convert -1 to short :) #168
Conversation
| @@ -52,7 +52,7 @@ class Graph(val schema: Schema, val storagePathMaybe: Option[Path] = None) exten | |||
|
|
|||
| /** Note: this included `deleted` nodes! You might want to use `livingNodeCountByKind` instead. */ | |||
| private[flatgraph] def nodeCountByKind(kind: Int): Int = | |||
| if (nodesArray.length <= kind) 0 | |||
| if (nodesArray.length < kind || kind < 0) 0 | |||
There was a problem hiding this comment.
The kind == nodesArray.length case is off-by-one and should be included in this check -- this ain't julia ;)
I would remove this check entirely: it is clearly an error and an immediate array-oob exception is best for debugging. But if we have the check then we should do it right.
There was a problem hiding this comment.
whoopsie, thanks for catching
There was a problem hiding this comment.
Updated - I also change the semantics: we now throw an exception if someone asks for an invalid kind, previously that would just return 0. I'm still undecided which way to go tbh... do you have a preference?
| @@ -3,7 +3,10 @@ package flatgraph.misc | |||
| object Conversions { | |||
| extension (i: Int) { | |||
| def toShortSafely: Short = { | |||
| assert(0 <= i && i <= Short.MaxValue, throw new ConversionException(s"cannot safely downcast int with value=$i to short")) | |||
| assert( | |||
There was a problem hiding this comment.
Again, I would not include such checks. But if you want to include it, then I guess i == i.toShort.toInt is more to the point.
There was a problem hiding this comment.
The reason for introducing this check is to provide a more specific error than an array-oob exception which is quite generic.
The .toShort.toInt check is equivalent but more clear, so yes good suggestion, thanks
There was a problem hiding this comment.
The reason for introducing this check is to provide a more specific error than an array-oob exception which is quite generic.
Yeah, but this is not an error condition that anyone is supposed to specifically catch and recover from.
It is an exception where somebody is supposed to stare at the stack-trace in an error log, look at the line numbers, and also look at the relevant code path.
Given that the person handling the exception already has that info, most manual throw error strings are pretty superfluous: For such exceptions, a source-code comment around the throw is equivalent to the static string part of an error message.
There was a problem hiding this comment.
and also look at the relevant code path
From your (and mine) perspective that's true, but that's one additional step that many downstream users simply won't go ¯_(ツ)_/¯
And in this specific case we do need the check, simply to avoid bugs that root in things like
scala> 3333333.toShort
val res0: Short = -9003
also: fix array kind bounds check