Run and cover compile-test instantiation #1092
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1092 +/- ##
===========================================
+ Coverage 90.42% 91.79% +1.37%
===========================================
Files 768 769 +1
Lines 63753 63826 +73
===========================================
+ Hits 57647 58592 +945
+ Misses 6106 5234 -872
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
OK this one was very difficult for me. It clears up a lot of chaff.
I left about 500 more lines in Together with the minimalistic This thingy needs to cycle green, then party. Cc: @jzmaddock and @mborland and @NAThompson |
|
My personal best (or worst) 14 failed CI runs 'till I dialed it. |
|
By the way, this drone run is getting so bad that it's almost worse-than-neutral. It fails so often, that I disregard it almost by self-preservatoin, thereby nearly missing the few things-of-substance it finds. I'll think on it, if there is a better way to reach some of those old/arcane compilers. Or we just have to seriously parse those outputs, which I do not like. I like green being green and red being red. |
|
Failures in drone are failures to obtain compiler |
| @@ -275,29 +276,30 @@ inline RealType quantile(const complemented2_type<cauchy_distribution<RealType, | |||
|
|
|||
| template <class RealType, class Policy> | |||
| inline RealType mean(const cauchy_distribution<RealType, Policy>&) | |||
| { // There is no mean: | |||
There was a problem hiding this comment.
Apologies for being absent while this was being developed, just trying to catch up now....
We really shouldn't remove the static_asserts here: the point is that the cauchy distribution has no mean, so calling mean on such a distribution is almost certainly a programming error that should be caught early (rather than at runtime). There are a small number of situations where we're writing generic software which "works with any distribution" where we can change the Policy as a "get out of jail free card" to convert the compile time error to a runtime one.
There was a problem hiding this comment.
I'm really glad to hear from you John. I was extremelty unsure about this. I came to the conclusion that (via default template parameters) the exact policy in question that was being used did not actually have any of the attributes needed for the static_assert.
These static_asserts are being asserted on defualt_policy, which is empty.
So that thing either needs content, or the static assertions are bogus.
I'm not entirely sure of this, but seem to have gleaned this from the code, ...
There was a problem hiding this comment.
To be clear, the default is that these functions do static_assert, we should have added a test to that effect.... somewhere lost in the depths of time, I remember Paul and I having a long discussion about this, the "obvious" thing to do was to simply not provide functions for mean etc when those are undefined, but that then prevents the option of writing generic code. So these need either a policy that contains assert_undefined<false>, or we need a #define BOOST_MATH_ASSERT_UNDEFINED_POLICY false.
There was a problem hiding this comment.
Ahh, instantate.hpp includes the define at it's start, but that will not be triggered if policy.hpp has been included first... so I'm guilty of some fragile programming there :(
There was a problem hiding this comment.
We could make the default policy have a bunch of constexpr stuff to pass the static assert tests. Or we could handle on the top layer with compiler switches.
I expect we will find several things of this nature as we press on toward ninety five, more percent.
This code base is awesome. Rarely, have I covered stuff with such depth and detail and established nature and found such deep thought going into it.
But we will find a few thingy-ies needing tweaks and patches toward, 93, 95, 98, 99
There was a problem hiding this comment.
Ahh,
instantate.hppincludes the define at it's start, but that will not be triggered ifpolicy.hpphas been included first, ...
Yeah! That is what I was missing. I studied that code for a while but did not dial that nuance.
Can we repair this in this running PR? Or should i take another run at it. I really got a good start at this thing...
There was a problem hiding this comment.
In other words @jzmaddock I'm looking to get about 1,500 lines covered if we can get these instantiations compiling and running in all situations. We are close. Maybe I need to include that policy and then restore those static_asserts. We are close to that in my latest PR.
| @@ -145,6 +145,8 @@ T bessel_y0(T x, const Policy&) | |||
|
|
|||
| static const char* function = "boost::math::bessel_y0<%1%>(%1%,%1%)"; | |||
|
|
|||
| static_cast<void>(function[0U]); | |||
There was a problem hiding this comment.
This can just be removed I think?
There was a problem hiding this comment.
I'm ok with syntax stuff. I tend to over-cast and handle warnings like a maniac. And thus (as some would say) obfuscate. I'll get back to this later after we hash out these basics.
There was a problem hiding this comment.
Last comment, I think some of those assrts are now known to be domain_error, not unknown.
| { | ||
| using std::fabs; | ||
|
|
||
| auto v_special { v1 }; |
There was a problem hiding this comment.
Why not just set v_special to something specific that's in domain for acosh? The fabs is unused as well.
|
|
||
| using std::exp; | ||
|
|
||
| while(v_special > exp(RealType(-1))) |
There was a problem hiding this comment.
As above, why not just pick a value?
There was a problem hiding this comment.
Sure. I appreciate that
| @@ -409,9 +467,21 @@ void instantiate(RealType) | |||
|
|
|||
| boost::math::unchecked_bernoulli_b2n<RealType>(i); | |||
| boost::math::bernoulli_b2n<RealType>(i); | |||
| boost::math::bernoulli_b2n<RealType>(i, i, &v1); | |||
| { | |||
| RealType* v1_array = new RealType[i]; | |||
There was a problem hiding this comment.
Some scoped storage would be good ;)
| boost::math::tangent_t2n<RealType>(i); | ||
| boost::math::tangent_t2n<RealType>(i, i, &v1); | ||
| { | ||
| RealType* v1_array = new RealType[i]; |
There was a problem hiding this comment.
What do you mean? Like std::vector?
There was a problem hiding this comment.
I was going to suggest a Boost solution, but these days it's spelled std::unique_ptr: https://en.cppreference.com/w/cpp/memory/unique_ptr
| { | ||
| using std::fabs; | ||
|
|
||
| auto v_special { v1 }; |
There was a problem hiding this comment.
As above, specific value.
There was a problem hiding this comment.
There are a bunch of those. I'm more concerned with how to get the exceptions and the instantiations running and compiling.
|
Before I get too far into specific details - what is the purpose of this change? Let me explain: the file instantiate.hpp was designed for one purpose only - concept checking. As a side effect, it also catches a few other things like missing Does this increase code coverage? If so, that's arguably a bad thing... let me try and explain, instantiate.hpp does just that, it instantiates things, it doesn't anywhere actually check that they do the right thing! In other words if there's missing coverage we shouldn't hide it behind something that's simply not designed for runtime testing, we should add some actual tests that validate behaviour. |
|
|
||
| const auto result_is_ok = (boost::report_errors() == 0); | ||
|
|
||
| return (result_is_ok ? 0 : -1); |
There was a problem hiding this comment.
Silly question: why not just return boost::report_errors() like everyone else? ;)
There was a problem hiding this comment.
Silly question: why not just return
boost::report_errors()
We can work out these syntactic details. Thanks.
Does this increase code coverage?
Yes it increases code coverage.
If so, that's arguably a bad thing.
To me this doesn't make sense. I get that there can be compile checks. But when the arguments are fixed, the code should run. Maybe I missed some syntactic details when I got the code running. Then I made a few mistakes (maybe) and the code does not compile. But i think it should do both.
We can hash out the syntactic details later, but I think the code in instantiate.hpp should both compile and run. Sure this might be a bit of a shocker in legacy code, but I would have a difficult time understanding why compiling exlcudes running (as long as the arguments are in range).
That's a good question. When I started, I wanted the code to actually run, not just compile. I don't exactly remember when CI started failing. But when I got the code running in some situations, it started failing to compile in others. This was confusing to me. So I started a series of efforts to get the code compiling and running in all situations. This ultimately led to an actual change needed in the standalone configuration. Then I kind of lost track of it. honestly I do not know or remember the actual change which led to the I think the code should compile and run. And my latest PR has this, except for one thing, the static asserts in |
|
I really do think the code in all situations should compile and run. This might be a philosophical difference, but I don't see why they should exclude each other. And this is where I got stuck. If I kick out those 7 static asserts in Cauchy and non-central beta, I get both. So when I get the code compiling and running in all CI, I can not find the needed syntax to get compilation working for those distributions missing such things as skewness (when I include the |
|
There must be a way to get both. I have not yet found it. But I'm close in 3801e07 |
The purpose of this PR is to cover the file instantiate.hpp.
The file instantiate.hpp has been extremely useful for compilation-syntactical correctness.
It is/was not all that far away from actually running with
boost::float64_t, which is exercised (with a few tens of modifications in the header) in this PR.