Convert assert to verify/test in math #259
Conversation
| @@ -295,13 +295,6 @@ public class Distribution ( T ) | |||
|
|
|||
| auto index = cast(size_t)(fraction * (this.values.length - 1)); | |||
|
|
|||
| assert(index < this.values.length, "index greater than or equal to length of value list"); | |||
|
Travis doesn't like it, though. |
|
|
david-eckardt-sociomantic
force-pushed
the
verify-math
branch
from
d4d95d1 to
56c0a0b
Compare
Codecov Report
@@ Coverage Diff @@
## v3.x.x #259 +/- ##
==========================================
- Coverage 71.83% 71.83% -0.01%
==========================================
Files 459 459
Lines 38978 38976 -2
==========================================
- Hits 28001 27999 -2
Misses 10977 10977 |
Codecov Report
@@ Coverage Diff @@
## v3.x.x #259 +/- ##
=========================================
+ Coverage 71.97% 72% +0.02%
=========================================
Files 458 458
Lines 38842 38855 +13
=========================================
+ Hits 27958 27979 +21
+ Misses 10884 10876 -8 |
The `if` condition is true if the preceding assertion fails. The `return` expression does the assertion again. The input value used for the calculation is checked by `enforce`.
…ions These functions are not supported on x86-64, all they do is to throw.
david-eckardt-sociomantic
force-pushed
the
verify-math
branch
from
56c0a0b to
1db9f60
Compare
|
Updated. |
| @@ -321,7 +322,7 @@ private: | |||
| tmpval &=0xFFFF_FC00; | |||
| asm { ld tmpval, %fsr; } | |||
| */ | |||
| assert(0, "Not yet supported"); | |||
| throw new SanityException("Not yet supported"); | |||
There was a problem hiding this comment.
Shall we simply deprecate these functions? It is weird to keep code that does nothing but throwing.
There was a problem hiding this comment.
Sure. We could even just remove them because they are literally dead code, no user code can possibly call them.
There was a problem hiding this comment.
Let's just follow usual procedure, no benefit in making exceptions :) New major should be soon enough for this to not matter.
|
Updated, deprecating functions that don't support x86-64. |
src/ocean/math/IEEE.d
Outdated
| @@ -307,6 +307,7 @@ private: | |||
| static assert(0, "Not yet supported"); | |||
| } | |||
| } | |||
| deprecated | |||
There was a problem hiding this comment.
Could someone with x86 assembly language knowledge take a look at resetIeeeFlags(), please ( @don-clugston-sociomantic pr @nemanja-boric-sociomantic )? It seems to me it would work with x86-64 but uses an orphan version identifier.
There was a problem hiding this comment.
It seems that that's still applicable even for x86-64 (as part of x87 FPU).
There was a problem hiding this comment.
That's right, there's no difference between x86 and x86-64 in this respect
david-eckardt-sociomantic
force-pushed
the
verify-math
branch
from
65ca10e to
f82c941
Compare
|
Updated, trivially extending |
|
Just an observation: there are two very different things that are replaced with It's a nice example of the naivety of D's assert system. Apart from that, LGTM. |
|
@don-clugston-sociomantic yeah we talked about it some time before in core chat - all ocean will really need another pass checking which |
|
@don-clugston-sociomantic, in addition to what @mihails-strasuns-sociomantic says, to address your concern I put the replacement of the different types of assertions in separate commits. See also my first PR comment. |
|
Cool, that sounds completely reasonable to me. In fact most of the floating point functions should probably return NaN on invalid input parameters, which would sidestep the issue in a lot of cases. |
I grouped them by topic:
assertmight actually be correct but I'm not perfectly sure.assert(0)tostatic assert(0)is for functions that support x86-64 but had anassert(0)for some other unsupported architecture.assert(0)tothrow new SanityExceptionfor disabled functions is for functions that don't support x86-64.static assert(0)cannot be used because the module wouldn't compile then. These functions are useless for us and should be removed (or fixed/extended to support x86-64 if it's worth it).