Pointer Parameter Problem

TL;DR

Go to the Summary

What I am talking about

Consider the following code:

int Foo(A* InA)
{
    return Bar(*InA);
}

Should this pass code review? I think most people will be thinking one of the 3 following answers:

1. Yes.
2. No.
3. Maybe/It depends. There isn’t enough information here.

I have recently been thinking about this. Without the ability to poll a wide range of people, and get actual figures, here is what I would predict to be the results of this poll (based entirely on conversations, and internet threads):

  pie
"Yes": 5
"No": 25
"Maybe/It depends": 70

With this context in mind, let’s have a chat about my motivations for this article!

Motivations for this article

I recently submitted code, that did the moral equivalent of the following:

#include <string>
#include <string_view>

void AppendString(std::string& InOutString, const char* InStringToBeAppended)
{
    InOutString += InStringToBeAppended;
}

std::string BuildString(const std::string_view InMiddle)
{
    std::string ret = "Begin";

    AppendString(ret, InMiddle.data());

    ret += "End";
    return ret;
}

std::string Get()
{
    return std::string{};
}

int main()
{
    auto str = BuildString(Get());

    return str.size();
}

The idea is that we grab some string, then build another string from that initial string. Simple. The initial string could be empty like in this example. Now, if you run this code on godbolt (just follow the link from the code example or here), you will find that no compilers see an issue with this code, and it will do what you would expect.

However, I am an Unreal Engine developer, and as such we do not use std::string and std::string_view. We use Unreal’s equivalents, FString and FStringView respectively. The following is a unit test that I wrote in Unreal Engine that demonstrates something similar:

And here is a table to help translate from the UE core library to the C++ standard library

I fully expected this to have the same behaviour as the example above. But it does not. This test does not even get to the ASSERT_THAT line. It crashes in FString::operator+=

The reason that the Unreal version crashes and the standard library version does not, comes down to how std::basic_string and FString handle empty strings.

Given T myStr{} where:

1. T = std::wstring -> Thanks to small string optimization, we get a valid empty null terminated string. Therefore calling std::basic_string<wchar_t>::data will always yield a valid null terminated string. And as a result std::basic_string_views constructed from a std::basic_string will always yield a valid string view that wraps a null terminated string.
2. T = FString -> FString is simply a wrapper around Unreal’s std::vector<wchar_t> (named TArray<TCHAR>) that has no small string optimization, so an empty string leads to an empty TArray with zero allocation. Therefore calling FString::Data can yield a nullptr when called on an empty FString. And as a result, FStringViews constructed from empty FStrings are not valid string views.

This distinction leads to the cause of the crash. We pass a nullptr to FString::operator+=. We can get the exact same crash with the standard library version if we change this line:

AppendString(ret, InMiddle.data());

to

AppendString(ret, std::string_view{}.data());

but thanks to the fact that default constructed std::basic_strings do contain a null terminated string, this will work just fine:

AppendString(ret, std::string{}.data());

We know where our crash is coming from. The question now is, how do we fix the crash? And herein lies the motivation for this article!

How should we handle pointer parameters?

This question really got me thinking about the ways in which we should approach functions that have parameters that are pointers. I think the way in which you approach this comes down to:

1. Function visibility -> Is this a public part of your API? Or is it a private implementation detail?
2. Performance -> Performance is always a concern in C++
3. The type of pointer -> Some pointer types are more special than others…

Let’s go through each of these properties and discuss them

1. Function visibility

If we go back to the initial code example of this article:

int Foo(A* InA)
{
    return Bar(*InA);
}

If Foo is a public function that can be called by anyone, then this fails code review for me, and I don’t think you would be able to convince me otherwise.

Since we accept the argument by pointer, we are explicitly stating that we can handle a nullptr scenario in a graceful manner. The above code clearly does not do that. Possible ways to change the code for me to pass it in code review are:

1. Return a std::optional<int> - We accept that it is valid that InA can be nullptr, and that returning 0 is very different from not being able to calculate anything at all.

2. Return 0 - It is valid for InA to be nullptr, and the semantics of calculating a 0 and receiving a nullptr as our argument are the same. So returning 0 is fine here.

3. Accept an A& - It is not valid for InA to be nullptr, so we therefore don’t allow callers to pass nullptr here.

Depending on the situation, any one of these above improvements would convince me to resolve the issue on the code review.

However, what if Foo was a private implementation detail? For me, it would still be a fail, and you would have to apply one of the above fixes. However, it would probably be reasonable for another code reviewer to be a little more lenient in this scenario. It could be the case that any function that calls this code must first validate that they are not passing a nullptr. This could be an implicit contract that is derived from the implementation. My argument to this line of thinking comes down to locality.

If I am doing a code review, I hate having to jump around the files in the review (or worse, having to jump around code in my local workspace) to validate that the code in a particular diff is correct. What I love, is if all the information to validate whether code is correct or not, is as close together as possible. So, if I have to find all the call sites of this private function just to determine if the call is correct, I will not be very happy.

Furthermore, if it is guaranteed to not be nullptr why not just accept it by reference? Changing that * to a & will make it so much easier to reason about for code reviewers, and you, when you read it again in a week, or 5 years.

Another option could be to use an assertion:

int Foo(A* InA)
{
    assert(InA);
    return Bar(*InA);
}

int Foo(A* InA)
{
    check(InA);
    return Bar(*InA);
}

int Foo(A* InA)
    pre(InA)
{
    return Bar(*InA);
}

But, again, why not just accept by reference instead?

2. Performance

This is C++ so performance is always an issue on people’s minds. Some could see redundant pointer checks as being an issue.

Let’s say we have the following code:

int Foo2(A* InA)
{
    int ret = 0;
    if (In)
    {
        ret = InA->GetAnotherThing();
    }
    return ret;
}

int Foo1(A* InA)
{
    int ret = 0;
    if (InA)
    {
        ret = InA->GetSomethingElse();
    }
    return ret + Foo2(InA);
}

int Foo(A* InA)
{
    int ret = 0;
    if (InA)
    {
        ret = InA->Get();
    }
    return ret + Foo1(InA);
}

In each of these functions we need to check if InA is valid. However, if we call Foo(notANullptr), then we are going to check if notANullPtr is nullptr 3 times. This is a little unfortunate. However, it is necessary, and with the way in which these functions are written there is no correct way around it. If we don’t do the nullptr checks in Foo1 and Foo2, then the functions have fundamentally different control flow. And in this case, we should just make them receive references like so:

int Foo2(A& InA)
{
    const ret = InA.GetAnotherThing();
    return ret;
}

int Foo1(A& InA)
{
    const int ret = InA.GetSomethingElse();
    return ret + Foo2(InA);
}

int Foo(A* InA)
{
    int ret = 0;
    if (InA)
    {
        ret = InA->Get() + Foo1(*InA);
    }
    return ret;
}

The issue with this is that we might have needed to call Foo1 and Foo2 as independent functions elsewhere, and we might have needed the fact that they handled the case of a nullptr. So how do we handle this case? We want to keep the nullptr branches to an absolute minimum, but at the same time, we want to support multiple independent functions that can handle nullptr. To solve this issue, we can separate the public interface from the private implementation:

// Private implementation

int Foo2Impl(A& InA)
{
    return InA.GetAnotherThing();
}

int Foo1Impl(A& InA)
{
    return InA.GetSomethingElse() + Foo2Impl(InA);
}

int FooImpl(A& InA)
{
    return InA.Get() + Foo1Impl(InA);
}

// Public interface
int Foo2(A* InA)
{
    if (InA)
    {
        return Foo2Impl(*InA);
    }
    return 0;
}

int Foo1(A* InA)
{
    if (InA)
    {
        return Foo1Impl(*InA);
    }
    return 0;
}

int Foo(A* InA)
{
    if (InA)
    {
        ret = FooImpl(*InA);
    }
    return 0;
}

Now we have the best of both worlds:

1. No matter which of the public functions we call, we will only get one branch checking for a nullptr
2. We can support multiple independent functions calling each other.

There is still an issue with this though, and that is combinatorial explosion:

// Public implementation
int Foo2(A* InA, B* InB)
{
    int ret = 0;
    if (InA)
    {
        ret += InA->Get();
    }

    if (InB)
    {
        ret += InB->Get();
    }

    return ret;
}

int Foo1(A* InA, B* InB)
{
    int ret = 0;
    if (InA)
    {
        ret += InA->GetSomething();
    }

    if (InB)
    {
        ret += InB->GetSomething();
    }

    return ret;
}

int Foo(A* InA, B* InB)
{
    int ret = 0;
    if (InA)
    {
        ret += Foo1(InA, InB);
    }

    if (InB)
    {
        ret += Foo2(InA, InB);
    }

    return ret;
}

In this example, we have 3 functions that take 2 pointer parameters. We need to support the fact that in each function there are 4 possible control paths to take, based on whether InA or InB are nullptrs or not. And if we call Foo, we will be doing redundant branches checking for nullptr. However, this can be remedied by breaking the logic down into smaller functions which operate on the individual valid A and B objects in isolation. This is moving towards an argument for smaller and simple functions. Simpler because they take fewer inputs.

This is all to say that when we are looking through the lens of performance, we can work around the potential issue of redundant checks for nullptr by ensuring that we separate the validation (branch on checking for nullptr) from the implementation (operations on the object once we have validated the pointer). In most cases this will be inlined by the optimizer. And if we are actually just going to assume that a pointer parameter is not nullptr, the advice is still the same. Accept it by reference.

3. Type of pointer

The past two sections have boiled down to the following bit of advice:

However, we can’t really do that when T is a const character type.

e.g. we can accept a const char& like so:

void AppendString(std::string& InOutString, const char& InStringToBeAppended)
{
    InOutString += &InStringToBeAppended;
}

And this would “work”, but, this would remove the ability for us to call the function with a string literal:

// cannot convert from 'const char [3]' to 'const char'
AppendString(ret, "Hi");

So what can we do instead? The answer is to accept by string view:

void AppendString(std::string& InOutString, const std::string_view InStringToBeAppended)
{
    InOutString += InStringToBeAppended;
}

void AppendString(FString& InOutString, const FStringView InStringToBeAppended)
{
    InOutString += InStringToBeAppended;
}

This works around the issue that both the standard library and Unreal Engine face. So essentially, we really should not be using const char* as a parameter type anymore. We should always be using std::string_view.

Fixing the crash

So with all of this in mind it might be quite obvious what I did to fix the crash. We changed our AppendString function to accept by FStringView. Or in standard library parlance, this

#include <string>
#include <string_view>

void AppendString(std::string& InOutString, const std::string_view InStringToBeAppended)
{
    InOutString += InStringToBeAppended;
}

std::string BuildString(const std::string_view InMiddle)
{
    std::string ret = "Begin";

    AppendString(ret, InMiddle);

    ret += "End";
    return ret;
}

std::string Get()
{
    return std::string{};
}

int main()
{
    auto str = BuildString(Get());

    return str.size();
}

For those who are a little more curious, the actual fix I submitted was to cherry pick this CL from UE5.1. If you don’t have access, you can link your Epic account to your github account to get access here. Sea of Thieves is on UE4.10 hence why we did not have this change available to us initially.

Should we stop there?

This was not my only thought on how to fix the crash… Both FString::Append and std::basic_string<CharT,Traits,Allocator>::append assume a valid pointer parameter in their const char* overloads. It is undefined behaviour to pass a nullptr to these functions.

And what that means is you will most likely crash at the point of the call to strlen.

Are we ok with this?

Consider what we have just discussed in the previous sections. Consider also, that everything that we have discussed is summarized quite nicely in the C++ Core Guidelines.

I, personally, am not ok with this.

To get rid of this sharp edge, I think there are two potential fixes:

1. Get rid of this overload of any function that also takes a string view in another overload:

2. Just do the nullptr check like we do with every other overload of these functions.

The issue with option 1 is that it relies on a string view implementation having an implicit conversion from const char*. To some this is fine. To others, implicit conversions are hideous monstrosities that should be avoided at all costs. I think that I fall in the “It’s fine” camp on this one, but I can see this being a hard conversation for some.

I have absolutely no aspirations for proposing a paper to fix this in the standard library… but Unreal Engine… That is something that is probably a little more achievable, and I was prepared to create a pull request for doing a nullptr check in FString::Append. In fact, that is where this unit test came from:

I wrote this test, to demonstrate my point that this crashes when it shouldn’t. There are two reasons why it shouldn’t crash:

1. Every guideline and good practice dictates that if we accept a pointer, we should assume a nullptr can be passed as an argument.
2. The semantics of passing a nullptr and an empty string are the same, in the context of append. Not convinced? Why does a std::string_view created from a default constructed std::string not crash when passed to std::basic_string<CharT,Traits,Allocator>::append?

Before starting to construct my pull request, I started a conversation with some folks to get their opinion on the matter. And the prevailing opinion was that they disagreed with me. There was the opinion that it is a programmer error and we should crash in this case. Honestly, the conversation went quite like this stackoverflow thread from 2010.

On one side you have people who are “vehemently” against checking for nullptr.

Part of why it’s become a religious war is that some of us have gotten stuck writing performance-critical functions that spend 75% of their time checking all their arguments against NULL and the remaining 25% performing a single trivial operation, due to legacy API requirements and/or broken callers that require NULL to be interpreted specially. As such, I am vehemently against any practice that perpetuates the myth among users of libraries that “passing NULL is okay” except when there’s a good reason for the function to accept NULL and it’s documented as such.

And then on the other end of the spectrum you have a somewhat more pragmatic argument.

“the callee is never responsible when the caller does something stupid” That is completely true, but there’s a big difference between what your code is actually responsible for and what your code actually gets blamed for.

I understand the issue of having to always check for pointers in a C API, and that doing that will add a performance hit, but this is C++. We have references, and string views. You check for nullptr once and everything after it can accept by reference. Or in this case, you could defer to the FStringView implementation. However, also because this is C++, any performance hit is going to be an uphill battle, even when correctness is involved.

Realistically speaking, there shouldn’t be much string processing occurring in performance critical code in a game engine like Unreal Engine, so I still think that it is worth following good practice here. But I also accept that I do not have the performance numbers to back that claim up… yet…

As for the standard library, I think there might be a sliver of hope with a proposal to fix APIs like std::basic_string<CharT,Traits,Allocator>::append. P2166 was accepted into C++23 which is getting quite close to the heart of this issue. However, I feel like the performance concern is where a proposal that aims to fix this issue would fall apart in front of the committee.

Summary

To sum all of this up:

  ---
title: What should my parameter be? Pointer or Reference?
---
flowchart TD
    start([Start]) --> IsStr{Is it a reference to a string?}
    IsStr -->|Yes| YesStr([Accept by string view])
    IsStr -->|No| CanBeNull{Is it valid to be nullptr?}
    CanBeNull --> |Yes| YesCanBeNull([Accept by pointer])
    CanBeNull --> |No| NoCanBeNull([Accept by reference])

Everything discussed here is consistent with the C++ Core Guidelines.

With modern C++, I don’t think there should be any exceptions to the above outside of having to interact with a C API. We just have to think a little more about our pre-conditions. “Is it valid for this to be nullptr?” should be a thought that goes through our minds a little more, and it should be something that we properly consider, rather than ignore it.

I am not entirely convinced by the argument of “performance”. “Reference correctness” should limit the amount of checking for nullptr to a minimum, and in the case of string literals, we have string views.

And finally, I don’t really think the standard library (or any other library) should be an exception to any of this either. There is probably a > 0% chance of a paper to remove undefined behaviour around nullptr parameters in the standard library succeeding.

There are likely cases that I have missed that might make me change my mind on this, so all thoughts and opinions are welcome. :)