cxx::function_ref, cxx::function and cxx::unique_ptr should not be nullable

[elfenpiff]

Brief feature description

The feature of a nullable function_ref and function can lead to a very easy misuse since one has to always check the function/function_ref if it is set before using it.

This causes:

• extra overhead
• increases the probability for undefined behavior/bugs

If one would like to have a nullable function/function_ref one can always wrap this into cxx::optional to make it clear that this is maybe null.

That this can happen very easily is demonstrated in the approved PR: #1094 (comment)

ToDo:

• function_ref
• function
• unique_ptr

[MatthiasKillat]

@elfenpiff Agree for function_ref (references should not be null and this is mainly for passing around, not storing), disagree for function as it is supposed to be stored and hence it is useful to be default constructible (which is only reasonable with being not callable (i.e. null), as done by std::function` as well).

This is similar to nullopt and I do not like to enforce wrapping it in an optional to achieve this (as usability suffers without gains). It is not a good idea to have function as a parameter type of a function.

Please remove this requirement from the issue as it limits useful designs and deviates more from the STL unnecessarily.

[elBoberido]

@MatthiasKillat if cxx::function can be null, you always have to do a if (functionVar) { functionVar(); }, right? If you don't do it how can you be sure not to call an uninitialized cxx::function?

[mossmaurice]

@elfenpiff I'm unsure if we should pursue this goal. I completely understand your intentions behind and even agree on them. However, we aim for iceoryx_hoofs being an independent, stand-alone library. With libraries, users will have a variety of use-cases we are currently not thinking about. Hence, when developing a library, we need a different mindset and need to take special care. This is always a consideration between restriction and flexibility and the decision of where iceoryx_hoofs shall go. IIRC this was also the reason on why we allowed a nullable function_ref in the first place as there where certain uses-cases which required a function_ref to be assigned during runtime.

Have a look at Folly's implemenation or at GNU's, both allow a nullable function_ref. We can take this discussion to our next developer meetup.

[elfenpiff]

@elBoberido @MatthiasKillat @mossmaurice what about the following approach. We create a generic free function which fits every interface and looks like:

template<typename ReturnType, typename... Arguments>
ReturnType genericEmptyFunction(Arguments&&...) {}

This function is by default always set for function and function_ref. Therefore both types are default constructible and are always valid without having an if check overhead. So it is safe to call the callable operator and it can never crash.

But then I would also remove the operator bool() from function/function_ref since it is not nullable in the classical sense anymore. What do you think?

[elfenpiff]

@mossmaurice Having a nullable function is a big mistake and we do not have to pursue the STL goal of introducing undefined behavior or make the life of users more complex by introducing error handling overhead where it is not needed.

You are right, we aim for iceoryx_hoofs of being and independent, stand-alone library - !for safety critical systems!. Why should someone use iceoryx_hoofs in a non safety environment where the STL with a lot more features is at their disposal?

But here I have to remind you, that @MatthiasKillat @mossmaurice @elfenpiff reviewed #1094 and none of us experienced developers did catch this - I just caught it by accident when I performed my final review. Having nullable types is dangerous and this PR proofed it and we are working in a safety critical environment!

[elfenpiff]

@mossmaurice Here are the design goals of Folly (taken from the Readme)

• library of C++14 components designed with practicality and efficiency in mind.
• Performance concerns permeate much of Folly, sometimes leading to designs that are more idiosyncratic than they would otherwise be

It reads to me that constructs are may more error prone or harder to use to just get more performance out. This is not something we should take inspiration from for safety critical systems!
Our goals would be:

• safety first, this means very hard to misuse, easy to use
• performance second (important but not when it compromises the first goal)

[elBoberido]

@mossmaurice as library developer we must be even more careful to provide constructs which cannot easily be used in a dangerous way, especially in out domain. With a null-able cxx::function_ref and cxx::function you impose the burden to check for null to all users, even if (out of thin air) 99% do no use it this way but since the type allows this, one has to check all the time if the object is null. This will even lead to poor performance since you need to check this everywhere. Making something null-able is a lazy solution. We should be more creative and we already have all the building blocks to do so. Let's wrap it into an cxx::optional and communicate when it can be null or if we really encounter a performance issue and it cannot be solved by improving the cxx::optional we can still introduce a cxx::nullable_function_ref or cxx::nullable_function. With this approach we do not need to litter our code base with pointless null checks just because in 1% (probably way less) of the cases it is required to be a null-able type. We can even have something like cxx::optional<cxx::function> nullable_function::to_non_nullable(); to convert a null-able type to a non-null-able type and be able to omit all the checks from that point.

BTW, when it comes to performance nothing beats GOTO. We should use GOTO everywhere ;)

[elfenpiff]

@elBoberido I totally agree with GOTO lets use GOTO everywhere.

[elBoberido]

reopen since function is still nullable