0

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;
Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574
Rezzie
  • 4,763
  • 6
  • 26
  • 33
  • @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
  • 1
    @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
  • 2
    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

7 Answers7

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

Community
  • 1
  • 1
Alok Save
  • 202,538
  • 53
  • 430
  • 533
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>();
fredoverflow
  • 256,549
  • 94
  • 388
  • 662
1

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

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
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.

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
0
delete NULL;

has no effect, so the test is not necessary.

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
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.

larsmoa
  • 12,604
  • 8
  • 62
  • 85
0

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.

C.J.
  • 15,637
  • 9
  • 61
  • 77