2

We have lots of existing code using raw pointers, which is littered with null checks at almost every usage.

In trying to write newer code more cleanly, we're trying to use factory constructor methods and unique_ptrs.

My question is, in the code below, once we've got our factory-created object - sensorX - can we use that throughout the rest of the code without further null checks on it, since it's a const unique_ptr?

DeviceFactory.h

class DeviceFactory
{
public:

    template<typename T>
    static unique_ptr<T> create(int id, std::string status)
    {
        auto device = unique_ptr<T>{ new T{ id, status } };

        if (!device) throw DeviceCreationException("device couldn't be created");

        return device;
    }
};

Usage

const auto sensorX = DeviceFactory::create<Sensor>(123, "sensorX");
Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
Artie Leech
  • 347
  • 2
  • 14
  • 4
    Use `std::make_unique` – MrTux Apr 05 '19 at 10:53
  • We're currently only allowed to use C++11. – Artie Leech Apr 05 '19 at 10:55
  • 1
    Then write your own `your_cool_namespace::make_unique`. You function is just the same as `make_unique` (only the `if (!device)` is redundant, it can't be null, cause `new` throws). No need to reinvent the wheel. [How to implement make-unique in C++11](https://stackoverflow.com/questions/17902405/how-to-implement-make-unique-function-in-c11). – KamilCuk Apr 05 '19 at 11:07
  • Your `throw` is redundant because `new` will throw if it fails to allocate. – Galik Apr 05 '19 at 11:24
  • 5
    (a) OP can't use `make_unique` so they write their own. (b) Contributor berates OP for not using `make_unique`. (c) OP points out they can't use `make_unique`. (d) Contributor instructs OP to write their own. (You couldn't make it up!) – Lightness Races in Orbit Apr 05 '19 at 11:48
  • 1
    If you feel like your code has more null checks than it ought to have, here is some guidance by Herb Sutter on pointers, smart pointers, references, and parameter passing. https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/ – Eljay Apr 05 '19 at 12:23
  • 1
    @Eljay, thanks for the Herb Sutter link - we'll obviously take those points on board for relevant scenarios. – Artie Leech Apr 11 '19 at 10:09
  • See also https://stackoverflow.com/q/55661068/94687 , where the author is "trying to create a not-null `unique_ptr`", and https://stackoverflow.com/a/46303030/94687 , which shows how "a `value_ptr` can be written that is not nullable. (Note that there are extra costs at move.)" – imz -- Ivan Zakharyaschev Feb 17 '20 at 06:11

5 Answers5

2

Looks like you wrote your own version of std::make_unique with a less flexible API. I would recommend aligning the API as it will make upgrades easier.

That said, your question is about the null-check. As indicated several times in comments, you don't need to check after getting a pointer, as std::bad_alloc should be thrown. However, let's assume that you another check that throws such custom exception, than the main question is: what's your API, including pre and post conditions.

In the codebase we work, std::unique_ptr ain't allowed to be nullptr unless documented. In that case: no need for a null-check. You could document it to be nullptr, or return an optional instead, which could indicate the invalid state.

My habit would be to assert on the pointer before usage or after creation. However, you can automate that work by using the sanitizers of Clang/Gcc. The GSL also has checking available for this nullptr usage.

My suggestion: assert after creating the variable (or use sanitizers), that should find the bug during testing if the post-conditions would change. While together with your colleges, you should agree that unique_ptr shouldn't be allowed to contain null unless documented what nullptr represents.

JVApen
  • 11,008
  • 5
  • 31
  • 67
  • Understood regarding this being a poorer version of make_unique, which we weren't aware of. "assert on the pointer before usage or after creation" - do you mean just once after creation, with the assumption that if const, it can't be set to nullptr? Or do you mean before EVERY usage, like Fowler's Guard Clauses? – Artie Leech Apr 11 '19 at 10:06
1

A unique_ptr can still be nullptr, it just means that this object handle the underlying resource, not that there is an acquired resource.

So you still need to check if the object exists or not, as you did before.

Side note: what forbids someone to call your factory and not store the result in a unique_ptr? As long as it's allowed, you will have to check.

Matthieu Brucher
  • 21,634
  • 7
  • 38
  • 62
  • "unique_ptr can still be nullptr" - Can it in my example above though, where I've already done an "if (!device)" check, and the created object is const? – Artie Leech Apr 05 '19 at 10:53
  • 1
    You are passing this unique_ptr as a const& everywhere? – Matthieu Brucher Apr 05 '19 at 11:04
  • yes, we'd expect to add const wherever possible, where it was declared const in the first place. Our toolset includes Klocwork and Resharper C++ which identifies areas where we can add const. – Artie Leech Apr 11 '19 at 10:08
1

You could do it even if it wasn't a const unique_ptr, provided you never reset it to null.

On your factory function,

        auto device = unique_ptr<T>{ new T{ id, status } };
        if (!device) throw DeviceCreationException("device couldn't be created");

the check in line two should be removed as simple plain new never returns null (well, unless you do something really awful to implementation defaults, I assume).

bipll
  • 11,747
  • 1
  • 18
  • 32
1

First of all the test:

if (!device) throw DeviceCreationException("device couldn't be created");

does not make much sense, because at the time when that if-clause is reached, it would not be possible for device to be nullptr (at least not for the given code)

From the perspective of the user, it is known that the function returns a unique_ptr due to the signature, but you need to read the documentation, if the DeviceFactory::create<Sensor> call guarantees that it does not hold nullptr. And even if the documentation would guarantee that, it might still be a good idea to test that yourself, because you don't know it that will be always the case in future.

So the question is if you always save it as const unique_ptr then why do you need a pointer at all, instead of writing:

template<typename T>
static T create(int id, std::string status)
{
    auto device = T{ id, status };

    return device;
}

But back to your question. If const auto sensorX = ... is initialized with a unique_ptr that does not hold nullptr, then it is guaranteed that it will be not nullptr for is lifetime.

t.niese
  • 39,256
  • 9
  • 74
  • 101
  • "So the question is if you always save it as const unique_ptr then why do you need a pointer at all, instead of writing:" It looks like I didn't really ask my question clearly enough, but your answer @t.niese is perfect for most of our scenarios. Knowing the Device is non-null and const at creation is what we're looking for. – Artie Leech Apr 11 '19 at 10:03
  • Good question: in this case, do by value. However, this ain't always possible when you have pure virtual methods or when you can't move (before c++17?) – JVApen Apr 11 '19 at 18:46
1

once we've got our factory-created object - sensorX - can we use that throughout the rest of the code without further null checks on it?

As other answers have indicated, the answer is "no". But what you can do is use the owner<> template (MS implementation), described in the guidelines:

  • One you've established a (raw) pointer to the factory-created object is non-null, wrap it in an owner<T*>;
  • Have ownership-taking methods take owner<T*> rather than T* as parameters.

This will guarantee, statically, that you've ensured ownershrip before making the call - so you don't need to check

einpoklum
  • 118,144
  • 57
  • 340
  • 684