Deleting an uninitialised object in C++

Question

In my header file I declare a variable within the scope of a class:

    FaultModel<double>   *fm_req_set_odom_px;

...which is conditionally initialised in the class constructor, depending on the value of a configuration file:

    const char *configModel = ConfigReader->ReadString("FaultModel");
    if (strcmp(configModel, "cyclic") == 0)
        fm_req_set_odom_px = new CyclicFaultModel<double>();

My question is: do I need to wrap the delete with a conditional to check if the model was initialised or not, or is it safe to just delete it in either case?

   if (fm_req_set_odom_px != NULL) // Is this necessary?
       delete fm_req_set_odom_px;

@Kerrek SB, you could collaborate a bit more about what resource managing containers are. It's not like everyone already knows C++11 by heart, and googling this doesn't really give a result — KillianDS, Jul 20 '11 at 10:03
@KillianDS: they've got nothing to do with C++0x; smart pointers and containers have been part of the C++ library since before there was a standard, and the advice to use them rather than trying to manage resources manually (which is always error-prone, and often impossible in the presence of exceptions) has been widespread for the last decade or two. — Mike Seymour, Jul 20 '11 at 10:21
Ah ok, it was not clear at all to me you meant smart pointers and the like ;). — KillianDS, Jul 20 '11 at 10:24
I didn't say "use C++11", I said "write C++ like we do in 2011, not like in 1994". The popular resource managing container is `shared_ptr`, which is available in C++0x, or as `std::tr1::shared_ptr` from `` in most C++98/03 compilers, or via ``. In C++98/03, `auto_ptr` is of some limited use, while in C++0x the `unique_ptr` is a lighter-weight container if you only have one single responsible party holding the pointer at any given time. — Kerrek SB, Jul 20 '11 at 10:37

score 6 · Answer 1 · edited May 23 '17 at 11:55

6

Apart from other answers which guide you appropriately,

If you MUST use dynamically allocated objects then do not use raw pointers but use Smart pointers.

Always make use of RAII(SBRM) it makes your life easier. This way you don't have to bother explicitly deleting any resources, the resources themselves will take care of it

edited May 23 '17 at 11:55

Community

1
1

answered Jul 20 '11 at 10:00

Alok Save

202,538
53
430
533

fredoverflow · Accepted Answer · 2011-07-20T11:34:16.613

5

delete NULL; is guaranteed to be a no-op, so a manual check is not necessary. However, an unitialized pointer variable is not NULL, so you must explicitly set it to NULL if the condition fails:

if (strcmp(configModel, "cyclic") == 0)
    fm_req_set_odom_px = new CyclicFaultModel<double>();
else
    fm_req_set_odom_px = NULL;

Alternatively, you can set the pointer variable to NULL unconditionally before the if statement:

fm_req_set_odom_px = NULL;
if (strcmp(configModel, "cyclic") == 0)
    fm_req_set_odom_px = new CyclicFaultModel<double>();

edited Jul 20 '11 at 11:34

answered Jul 20 '11 at 09:55

fredoverflow

256,549
94
388
662

5

The OP never said that she initialized the pointer to 0, though, which is the more glaring flaw in her code. – Kerrek SB Jul 20 '11 at 09:56
2

IMHO you should use the C++0x nullptr, the NULL macro is no longer fashionable :) - edit, Kerrek's comment is valid, if the pointer is not initialized to nullptr it is NOT safe to delete it. – Joris Timmermans Jul 20 '11 at 10:03
Which I could vote more than once on what Kerrek SB said... – C.J. Jul 20 '11 at 11:27
Yay - or even, *initialize* the pointer variable to NULL :-) – Kerrek SB Jul 20 '11 at 12:04

Nim · Answer 3 · 2011-07-20T10:13:32.557

1

There is no need for the check. But better approach is to use a smart pointer...

edited Jul 20 '11 at 10:13

answered Jul 20 '11 at 09:54

Nim

33,299
2
62
101

2

What harm do you believe that using `delete` with a null pointer does? Because it doesn't. – Steve Jessop Jul 20 '11 at 09:56
I won't remove my answer as I clearly say that using a smart pointer is the better approach, but I stand by the check... – Nim Jul 20 '11 at 10:02
1

The check does little harm, just bloats the source code. What you said was, "you should always check before deleting". Even if the check does no harm at all, it does no good, so I don't see why everyone should always do it. – Steve Jessop Jul 20 '11 at 10:05
@Steve... fine - it's my habit, and it's not hurt me, but if it's such a painful sticking point, I'll remove the first sentence... – Nim Jul 20 '11 at 10:12
@Nim: I removed my comments now that you edited your post. – Kerrek SB Jul 20 '11 at 10:34

score 0 · Answer 4 · answered Jul 20 '11 at 09:53

0

Typically, you would instead use something like a boost::variant to hold the possibilities. Heap-based dynamic allocation is wasteful in this case. boost::variant will always appropriately destroy it's contents for you and you won't have to worry about writing your own copy constructor/assignment operator.

answered Jul 20 '11 at 09:53

Puppy

144,682
38
256
465

1

boost::optional. -- I can't think of the good reason for offering boost::variant but not boost::optional in this particular case – sehe Jul 20 '11 at 09:58

score 0 · Answer 5 · answered Jul 20 '11 at 09:54

0

delete NULL;

has no effect, so the test is not necessary.

answered Jul 20 '11 at 09:54

qbert220

11,220
4
31
31

but what if the pointer is never initialized to 0 or NULL in the first place? – C.J. Jul 20 '11 at 11:26
@CJ: `delete NULL;` always has no effect. NULL is always NULL - there is no pointer to initialise. `delete p;` where `p` is not initialised is UB. – qbert220 Jul 20 '11 at 11:41

larsmoa · Answer 6 · 2011-07-20T11:32:13.007

0

delete checks for NULL, there is no need for if (myVar != NULL) delete myVar;. Also, consider using std::auto_ptr (or some other smart pointer container). This gets you out of a lot of troubles, e.g. uninitialized pointers.

edited Jul 20 '11 at 11:32

answered Jul 20 '11 at 09:56

larsmoa

12,604
8
62
85

score 0 · Answer 7 · answered Jul 20 '11 at 11:24

I think a more fundamental question to ask first is: Are you initializing the pointer to NULL in your constructor? (i.e. using an initializer list for instance).

If you are not initializing it to NULL, then you will most likely get access violations as you attempt to delete random bits of memory. If you are initializing it to NULL, then you can skip the if guard statement.

Deleting an uninitialised object in C++

7 Answers7