strings like ".Exxx" are not valid floats/doubles#6726
Conversation
| val e = format.charAt(noSignificant) | ||
| if (e == 'e' || e == 'E') expOK(noSignificant + 1, endIndex) | ||
| else false | ||
| } |
There was a problem hiding this comment.
Speaking of formatting, no one mentioned the variant space after if. This code base is mostly but not absolutely for the space, which is my preference; but a couple of people omit it. Probably it's best to pick one and gird for battle.
There was a problem hiding this comment.
I tend to be fully consistent with the rule that I use, like, whatever feels right at the time, man.
But considering I've annoyed you several times in the last week already, and I don't want to stay consistent in that regard, I'll put in some spaces.
There was a problem hiding this comment.
i.e. (e == 'e' || e == 'E') && expOK(...)
There was a problem hiding this comment.
I read that we're going for parseDouble compatibility. Just to mention that Scala doesn't take 5.E2 as a constant anymore, because of the ambiguity with selection.
There was a problem hiding this comment.
And then:
noSignificant != startIndex + 1 && {
val e = ???
(e == 'e' || ...) && expOK(...)
}
There was a problem hiding this comment.
@som-snytt This entire validation monster returns a boolean and is mostly made out of expressions that return booleans, so I could make it all in to a vary long series of nested and chained && and || 's. This fragment could be
(startChar == '.' && {
val noSignificant = skipIndexWhile(ch => ch >= '0' && ch <= '9', startIndex + 1, endIndex)
(noSignificant != startIndex +1)
} && {
val e = format.charAt(noSignificant)
(e == 'e' || e == 'E') && expOP(noSignificant + 1, endIndex)
} || (...)
But the explicit if/elses with boolean literal returns in some places make it much easier to follow for me here. There probably is some arbitrary line where opinions can differ. I have no strong feelings where that line should be, so if there are particular places where you'd like to see it moved, I'm fine with that.
There was a problem hiding this comment.
Yes, there is a line of taste. Returning true might be a code smell, but I trust your judgment.
| if (noSignificant >= endIndex) true //no exponent | ||
| else { | ||
| val e = format.charAt(noSignificant) | ||
| (e == 'e' || e == 'E') && expOK(noSignificant + 1, endIndex) |
There was a problem hiding this comment.
Here you didn't if-else it, so I would boost the other expressions, too,
| } | ||
| else if(afterIntChar == 'e' || afterIntChar == 'E') expOK(noInt + 1, endIndex) | ||
| else if (afterIntChar == 'e' || afterIntChar == 'E') expOK(noInt + 1, endIndex) | ||
| else false |
There was a problem hiding this comment.
Like this if-else is exp1 || { val e = ???; exp2 } || exp3.
som-snytt
left a comment
There was a problem hiding this comment.
I think some of the if-else make the code read more imperative than necessary, when a boolean expression suffices, assuming it doesn't become too unwieldy.
It was neat to see scalacheck at work. It's also interesting to read the history of the API upgrade.
som-snytt
left a comment
There was a problem hiding this comment.
Thanks! You may have used extra parens, but who's counting?
|
Could this explain the new test failure we’re seeing in #6635? |
|
Yes, there's already a PR #6746 |
Fixes scala/bug#10925
Should fix the test failure on #6559
Added the failing testcase that was generated by scalacheck, and a few close variations.
Make test failure message on the scalacheck tests clearer