cmake, more tests, unique header and thread-safe #3
Conversation
|
Thanks. cmake integration would be great. I'll review the changes. |
There was a problem hiding this comment.
Thank you for the PR, but I do not merge the addition of calc() method. I like the cmake support and the demos, so I welcome PRs or issues for adding them! Comments follow.
CMake, tests and GitHub workflow
Please do not build multiple test executables. It is time-consuming to compile a code including Catch2 with CATCH_CONFIG_MAIN. I'd like to isolate main code into main.cc and write tests in other source files, then link objects into a single executable to save compilation time. (See Catch's tips page.) It is fine to split tests into multiple source files.
The PR removes the workflow file, but I need it! Especially when reviewing a PR. I want to check for bad commits on GitHub.
The PR removes the vendored catch.hpp and requies Catch to be installed on the system. Is this the best practice? I have no idea. I included a copy of Catch so that external updates do not suddenly break tests. In the past Catch v1→v2 transition broke my tests, and currently v2→v3 transition is ongoing. Maybe we need to investigate what other projects using Catch do. Or migrate to doctest.
calc() and mutex
I don't think we need to add calc() method and a mutex to cubic_spline. Updating a spline always results in a full re-calculation, so in-place updating makes little sense. You may equivalently construct a new object and assign it back to a variable:
{
std::lock_guard<std::mutex> guard(spline_mutex); // if you need lock
spline = cubic_spline(t, x, cubic_spline::natural);
}This way we can keep the constness of operator() and also can avoid the cost of mutex in non-threaded applications. If you find the manual locking cumbersome, you should create a locking wrapper class without changing the cubic_spline class.
Constructor
In this PR, the constructor of cubic_spline is removed in favor of calc(), which makes cubic_spline default constructible. Member variables are initialized. But they are not in a good state! Calling operator() on a default constructed object triggers an undefined behavior, accessing _splines.front() or _splines.back() on an empty _spline. This is bad.
I think spline interpolator should not be default constructible (i.e., one must have a constructor) because it does not have a well-defined empty state. It's better to use std::optional<cubic_spline> if one needs lazy initialization.
Getters
The PR adds getLowerBound() and getUpperBound() member functions. These getters can be useful! I follow the style of the standard library, so these functions should be named lower_bound() and upper_bound(). Also, getter functions should be const.
Demos
Demos are welcome! A minor nitpick: if you use stdio, please include <cstdio>.
CMake, tests and GitHub workflow
Please do not build multiple test executables. It is time-consuming to compile a code including Catch2 with CATCH_CONFIG_MAIN. I'd like to isolate main code into main.cc and write tests in other source files, then link objects into a single executable to save compilation time. (See Catch's tips page.) It is fine to split tests into multiple source files.
I know it takes longer to compile but the catch doc is totally opaque and honestly zero to time to learn it.
Feel free to speed things up. I'm getting the idea. I tried it. Was utterly frustrated. I'm in the middle of term so time is precious.
Compile time not so much! ;)
The PR removes the workflow file, but I need it! Especially when reviewing a PR. I want to check for bad commits on GitHub.
Feel free to add it back in. I just realised it needs updating and again no time to understand what's going on.
The PR removes the vendored catch.hpp and requies Catch to be installed on the system. Is this the best practice? I have no idea. I included a copy of Catch so that external updates do not suddenly break tests. In the past Catch v1→v2 transition broke my tests, and currently v2→v3 transition is ongoing. Maybe we need to investigate what other projects using Catch do.
I personally never include another source if there is a package available. The other option is to do a github checkout on the fly for the Windows
people. Tried it. Failed miserably. Again. Catch docs are horrendeous. At the end it's just a bunch of compare operations.
I don't think we need to add calc() method and a mutex to cubic_spline. Updating a spline always results in a full re-calculation, so in-place updating makes little sense. You may equivalently construct a new object and assign it back to a variable:
{
std::lock_guard<std::mutex> guard(spline_mutex); // if you need lock
spline = cubic_spline(t, x, cubic_spline::natural);
}
Totally getting your point. I have removed all mutex related stuff. I have updated the "repeated test" to see if the copy constructor works
that's I need in my code:
spline = cubic_spline(knots2, values2, cubic_spline::not_a_knot);
...and it works nicely. See my iir1 library where we had a lot of grief with that but here it works out of the box.
Basically all calc can run in the constructor so I've just do that which should have it now as fast as it can.
This way we can keep the constness of operator() and also can avoid the cost of mutex in non-threaded applications. If you find the manual locking cumbersome, you should create a locking wrapper class without changing the cubic_spline class.
Constructor
All locking removed. The operator() is const again.
In this PR, the constructor of cubic_spline is removed in favor of calc(), which makes cubic_spline default constructible. Member variables are initialized. But they are not in a good state! Calling operator() on a default constructed object triggers an undefined behavior, accessing _splines.front() or _splines.back() on an empty _spline. This is bad.
I think spline interpolator should not be default constructible (i.e., one must have a constructor) because it does not have a well-defined empty state. It's better to use std::optional<cubic_spline> if one needs lazy initialization.
Getters
See above. I got carried away for my application as I need to assign an instance multiple times but I can just do:
spline = cubic_spline(knots2, values2, cubic_spline::not_a_knot);
as many times as I like and put a mutex around it if I need it. Not in your lib.
The PR adds getLowerBound() and getUpperBound() member functions. These getters can be useful! I follow the style of the standard library, so these functions should be named lower_bound() and upper_bound(). Also, getter functions should be const.
Fixed. No longer camel case.
Demos
Demos are welcome! A minor nitpick: if you use stdio, please include <cstdio>.
Changed.
|
Thanks for the detailed feedback. See my replies in the commit. |
|
BTW that's your library in action: https://www.youtube.com/watch?v=7os9slgfJcU |
btw that's the reason why I've split it up. cmake allows a nice structuring. otherwise it would just report "Test 1 OK" if it were on file. |
Explained here: https://catch2.docsforge.com/v2.13.2/faq/slow-compiles/ So now all good with catch compiling as fast as before as it was one file.
Of course the default constructor makes no sense except for allocating an initial instance of the spline in a constructor for example.
Great library. I need it to animate some graphics from data streaming in. The spline is constantly updated and it's done from another thread. I've added more tests for it and generally added cmake as a built system. Gave it also a unique name so that a "make install" won't clash with other libraries.