-2
\$\begingroup\$

I am not sure whether it is the right way or am I deleting the memory properly. I have seen here some custom implementation of linked list, I want to do it simple way. Can anyone please suggest is it right or not? Should I use shared ptr, even if I do it, how to implement it correctly.

.hpp

class Camera {
    Camera()
    {}
    ~Camera()
    {}

}; typedef struct Node *NodePtr;

// Now declare the structure type itself
struct Node
{
    Camera_ x;
    NodePtr next;
};

.cpp

NodePtr head = NULL;
NodePtr* ptr = &head;
for (int n = 0; n<n1; ++n)
{
    *ptr = new Node;
    (*ptr)->x = Camera(); //storing camera objects
    ptr = &(*ptr)->next;
}

// terminate the list.
*ptr = NULL;

// walk the list,
for (NodePtr cp = head; cp->next != NULL; cp = cp->next)
{
    // doing camera functions

}
\$\endgroup\$
5
  • \$\begingroup\$ Are the cameras all identical objects? It's not clear what code is to be reviewed, as it looks like you have presented a sanitized version of your real code. \$\endgroup\$ Feb 7, 2017 at 17:27
  • \$\begingroup\$ I want to know is it the right way? No , there would be different camera objects. \$\endgroup\$
    – Angelina
    Feb 7, 2017 at 17:43
  • \$\begingroup\$ why not std::list? \$\endgroup\$
    – pm100
    Feb 7, 2017 at 17:54
  • \$\begingroup\$ typedefs like NodePtr are an abomination: They just add more mental overhead. \$\endgroup\$ Feb 7, 2017 at 18:55
  • \$\begingroup\$ Check for cp != NULL instead of cp->next != NULL. \$\endgroup\$
    – G. Sliepen
    Feb 7, 2017 at 20:15

1 Answer 1

2
\$\begingroup\$

if you dont want to use the c++ standard data structures then here are a few comments

for (int n = 0; n<n1; ++n)
{
    *ptr = new Node;
    (*ptr)->x = Camera(); //storing camera objects
    ptr = &(*ptr)->next;
}

First, you are storing local (stack) camera objects in your linked list. This is asking for trouble if you try to do more with the list than use it all in one function. The stack memory will be released on function exit

You never delete the Node objects that you create. This is bad. You need a Node destructor plus you should encapsulate the list in a class that has a destructor too.

Or just use std::list

You also refer to a type Camera_, this is not defined - I assume this is a typo

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.