-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensuring non-null arguments #37
Comments
@gflegar wrote:
Alas, there is no standard C++ library way of doing that at compile time. For doing this at run time, a class like your I understand |
I'm not sure I understand. It shouldn't be a different kind of pointer. If it was, then template <typename Pointer, typename = void>
class non_null : public typename std::remove_cv<Pointer>::type {
public:
template <typename... Args>
non_null(Args...&& args) : Pointer(std::forward<Args>(args)...) {
if (*this == nullptr) {
throw std::runtime_error("Expected a non-null pointer");
}
}
};
// specialization for plain pointers (C++11, templates are uglier than in C++17)
template <typename Pointer>
class non_null<Pointer,
gko::void_t<typename std::enable_if<
std::is_pointer<Pointer>::value
>::type>> {
public:
using pointer = typename std::remove_cv<Pointer>::type;
using value_type = typename std::remove_pointer<Pointer>::type;
template <typename... Args>
non_null(Args...&& args): p(std::forward<Args>(args)...) {
if (p == nullptr) {
throw std::runtime_error("Expected a non-null pointer");
}
}
// implementations of get(), operator *() and operator ->()
}
private:
pointer p;
}; Both versions have similar interface, but I tend to like the inheritance one more. We do need a separate class specialization for plain pointers, but the version for smart pointers works like a charm, and propagates whatever interface |
If you think about it, it's impossible to do at compile time anyway. You only know what the pointer points to at run time. |
I favor use of references for nonnull input arguments, mainly because
It's your library, though :-) . There's no serious performance issue here for us. We would end up using this library mostly for large sparse objects anyway. |
I still don't see how references would help with this? |
This is a good point. It's possible to add this compile-time checking to non_null(std::nullptr_t) = delete; |
I must say I like keeping pointers. The decorator looks fairly good and easy enough to put in place without too many changes, in addition the last answer with the deleted constructor for It's maybe a personal preference or misunderstanding, but since all objects we use are dynamic memory I like to use pointers to represent them. Even though the memory is managed automatically, I think everyone (even people coming from other languages) get a good feeling for what happens behind them due to that. And mixing both uses is definitely clumsy. |
@tcojean @hartwiganzt just want to remind you of this one. It's quite an important feature, which should not be that difficult to implement (there's a prototype implementation above), but constitutes quite a big interface change (true change, not an addition) from the user's perspective. This means that if we do not do it now, it has to wait until Ginkgo 2.0.0. |
Somewhat related is also #179. |
The problem
It almost never makes sense to pass a null pointer to one of Ginkgo's methods. There's usually no way to continue and the best we can do is just throw an exception. However, we don't ensure this behavior anywhere yet, so passing a null pointer will result in a crash when we try to dereference it.
Usual solution
The proposed C++ way (Sutter's Mill, also suggested by @mhoemmen) is to use a reference (
&
) instead of a pointer for objects that cannot be null (assuming we are not giving any onwership).I must say I don't like this approach as it has some drawbacks (in the order of importance, so if you are convinced by statement
n
, you can skip statementsm > n
):uniqe_ptr
orshared_ptr
, just a plain pointer.->
we need to start using.
). This non-uniformity makes it more difficult to write code that doesn't depend on the kind of ownership we have (and may be confusing for some inexperienced users).gko::clone
utility proposed in More descriptive memory management #30. While we only need a single implementation, that internally callsobj->clone()
independent of what kind of ownership the object has, supporting references would require writing another version of the method that callsobj.clone()
instead.void f1(LinOp &x)
, and the second thingvoid f2(LinOp &y)
. I didn't change the functionality, but I have to find and replace allx->something()
withx.something()
just because I want to assert thatx
cannot be null.x
, and "thing 1" should have a temporary copy of 'x'. I decide to addauto tmp = gko::clone(x)
, and usetmp
instead. Now I sure have to replace every occurrence ofx
withtmp
, but I also have to change how I call things ('.' with '->'), which can be a bit confusing.Proposed solution
Rather than mixing references/pointers just to express if the argument can be null, I would suggest adding a decorator that throws a runtime error if the object is null. Here is the pseudocode (that would probably have to be refined a bit to handle special cases, e.g. plain pointer):
We could also publicly inherit from
Pointer
instead, to provide all the same methods as Ponter does (we have to be careful not to inherit from plain pointer).Then we could use:
I think it's pretty self documenting that all these expect a non-null pointer, maybe even more than by passing by reference, and we get a run-time check that the pointer is actually not null.
The text was updated successfully, but these errors were encountered: