Naming of "safe" versus "fast" functions

I would prefer to know when to use one vs the other.

Oh sure you could name one list_push_fast() and the other list_push_safe() but as your newest on-boarded employee I've no idea when to use one vs the other.

First, understand that removing two null checks is an exceedingly trivial optimization. Speed is a poor justification for separating validation. A better justification is that validation has already been performed elsewhere.

So if, as my new boss, you insisted on keeping both I'd name them list_push_checked() and list_push_unchecked(). If we name one simply list_push() my vote is to make that the checked one.

In a scenario like this, there are not just two, but at least three sensible ways the function could be designed:

1. If pointer is null, function's behavior is specified as doing nothing.

2. If pointer is null, function's behavior is specified as trapping.

3. If pointer is null, function's behavior is undefined.

In some similar scenarios, it may make sense to have semantics that would be specified as either trapping if observable nonsensical behavior would be otherwise unavoidable, but allowing nonsensical behavior to be avoided without a trap. For example, a function which is supposed to compute x*y/z with integer values might guarantee that it will either yield an arithmetically-correct value or trap, but not specify which option it will choose if x*y would overflow. Under such a specification, a function could optimize the case where y==z and z!=0 so that it simply returns x without having to determine whether x*y would overflow.

A variant that can't guarantee anything about corner-cases behavior would be "unsafe" compared with one that could offer some behavioral guarantees, but whether a function would be "safe" in a particular use case would depend upon the caller's requirements. Consider, for example, a malloc() style function. Depending upon circumstances, a function which is specified to always either yield a valid pointer or trap may be "safer" than one which would return null in case of allocation failure, or the fact that the function might trap could mean that it's "unsafe" and the version which would return null would be safer, despite the fact that the caller would need to validate the result.

I'm assuming that you have a more complex usecase and you're not trying to optimize the code actually posted (which is 2 cmp statements and if that needs to be optimized out you're likely to find more speed elsewhere). I would explicitly call one version unsafe or unchecked as other suggested.

I would also suggest putting a lint rule into your build. That lint rule should consider any use of the fast version an error. You can then use a lint disable comment on lines where it's actually needed. This will ensure that users don't blindly use the unsafe version and put a little thought into it.

Try to make the function fast and safe.

C++ references

In C++ you can use references when the object must be present (there is no null references).

void list_push(list_t &ref, list_t &item) { // ref and item can't be null, no need to check item.next = ref.next; ref.next = &item; } int main() { list_t list, item; // list and item is guaranteed to be there, null checks are a wastefull (but // probably optimized away). list_push(list, item); list_t *pitem = try_get_item(); // try_get_item() can fail, so null check is needed before dereferencing. // Note that the check is only done in the caller, where it can be handled properly. if (pitem) { list_push(list, *pitem); } } 

Use the type system

For example, use not_null from CppCoreGuidelines:

// The function interface now says that it do not want nulls void list_push(not_null<list_t *> ref, not_null<list_t *> item) { // ref and item are asserted to not be null in the not_null constructor. // The assert can often be optimized away. item->next = ref->next; ref->next = item; } 

Use asserts

Available in most languages. Will often be optimized away. Can often be removed completely in release builds after testing.

void list_push(list_t *ref, list_t *item) { assert(ref && item); item->next = ref->next; ref->next = item; } 

In C, in this particular case, you don't.

Pushing a null node onto a null list is a bug. No matter what you do, there will be a bug. Returning an error code doesn't make it not a bug. Returning an error code means the program keeps running, but now there is an item missing from a list. Which item? Which list? I don't know that. Only the caller knows that. It could be the list of nuclear weapons to disarm, and it's a bad idea to just skip one.

Does every "safe" function caller check the error code? If so, is that easier than checking that the parameters aren't NULL? If not, then why not check that the parameters aren't NULL, and then you can have one function that is either fast or safe depending on whether you check for NULL first?

If you want to guarantee fail-fast behaviour, you can also assert inside the function that the parameters are not NULL.

Besides, why NULL specifically? What if someone passes (list_t*)14? Or a dangling pointer?

There are two possibilities: One is that speed for this code is of no importance. Have you measured it? If not, no importance. Use the safe code. Two is that speed is important, in that case take the safe function and declare it as "inline" or "static". Obviously not calling the "fast" function but incorporating it. And you would check your compiler documentation if there is a way to indicate which result is expected, for example if you expect that in 99% of all cases the two pointers are not NULL.

Another possibility is that calling the function with a NULL pointer is a severe bug. In that case the caller is unlikely to have code checking the return value, and you may be better off using assert or similar to check that the pointers are not NULL. And if you find they are, fix the caller.

On the other hand, you might not even need an assert because your program will crash / fall into the debugger if one of the pointers is NULL.

If possible you can separate the actual activity and the checks into two different components. One wraps around the other and external components that want to be always safe use the safe wrapper while components that are so deep in the system all parameters are already validated at that point just go with the inner plain class that holds the unprotected implementation.

Obviously this does not work for all cases, but it might be worthwhile to consider.

In this case the "safe" option is still unsafe when you pass in pointers to invalid memory. The only way to have it safe is to call the function correctly. This is true for both options. Especially for C or C++ code, I'd rather have the unchecked option as the default than to have it do some checks that may or may not make it safer by a small amount and claim a false impression of fail safe behavior.

If your use case for the "safe" option is really some function returns an object or null, that I want to push to the list directly if available, then I'd suggest to add the checked variant as a special case. Name them after the use case: list_push_if_not_null().

Note that the return value in case nothing is added isn't actually an error here. It's the expected and successful behavior in case null is given. Consider to omit that return value. If you want to use that info for control flow external to that function, you might want to perform that check external to the function too.

Checking if the first parameter is null doesn't seem to make sense to me here. This is not Java. In C a non-zero pointer might point anywhere. You should rather add a test suite to the client code to make sure you call the function with an actual object of the required type. As a library author you might want to add documentation and example code though.

Also consider to add a different type for list items. For your own sanity. And note that names ending with _t are reserved by the POSIX standard. So if this is your actual code, you might want to rename your types.