Make shift list to modify pointed to objects

[fcarney]

I originally wrote this code using QObjects. But for clarity I just went with int*.

    int* ptr1 = new int;
    int* ptr2 = new int;

    for(auto ptr: {&ptr1, &ptr2}){
        if(*ptr){
            // call something on int*
            delete *ptr;
            *ptr = nullptr;
        }
    }

I had a couple of pointers I wanted to do some simple manipulation before deleting them. I find it handy to just make a list of values on the fly and use a simple for loop. This list used pointers I wanted to modify so I had to use indirection. The amount of code I saved was trivial. I just hate duplicating code. I think the indirection makes it harder to understand at first glance. But I feel like a c++ programmer should know how this works in general. I like using this method to handle short lists of data that will need identical treatment and when I don't feel like making a lambda or a function.

Is this just horrible code smell?

[Chris Kawa]

Is this just horrible code smell?

You're creating a data structure by copying values on the fly just so you can use a language construct that you like (for loop). Kinda wasteful. Horrible? Well, I've seen worse, but It does stink a little, yes.

Btw. your code has some bugs. Should be:

delete ptr;
ptr = nullptr;

otherwise you're trying to delete garbage at address represented by that int and then writing 0 to the memory the pointer points to and not nulling the pointer. Also, you probably meant if(ptr), which checks if the pointer is non-null and not if(*ptr), which checks if the value pointed to is non-zero.

[fcarney]

Its a double pointer. You have to do that to modify the original pointer when in the data structure.

Edit: That is why I asked about readability.

auto ptr:  should probably be int** ptr: to be more clear

[Chris Kawa]

@fcarney Hah, right, I took too fast of a glance. Well, there's your answer. It doesn't help readability ;)

[fcarney]

I originally did the code this way:

    int* ptr1 = new int;
    int* ptr2 = new int;

    for(auto ptr: {ptr1, ptr2}){
        if(ptr){
            // call something on int*
            delete ptr;
            ptr = nullptr;
        }
    }

But this modifies the copy of the pointer in the structure. Not the original pointer. This is a bug to write it this way.

[fcarney]

@Chris-Kawa I changed the code to be explicit ** in my code. auto is too confusing.

[Christian Ehrlicher]

Work on the reference:

@fcarney said in Make shift list to modify pointed to objects:

for(auto &ptr: {ptr1, ptr2}){

[fcarney]

@Christian-Ehrlicher said in Make shift list to modify pointed to objects:

Work on the reference:

That wont work if I am trying to null the original pointer. The pointer in the make shift object (struct?) is a copy.

[Chris Kawa]

@fcarney That's 0:2 in the game of readability vs this code :)

[J.Hilk]

@fcarney you know this is also an option:

int* ptr1 = new int;
    int* ptr2 = new int;

auto doStuffWith = [](int *ptr){
 if(ptr){
            // call something on int*
            delete ptr;
            ptr = nullptr;
        }
};
  doStuffWith(ptr1);
 doStuffWith(ptr2);

[fcarney]

@J-Hilk said in Make shift list to modify pointed to objects:

auto doStuffWith = [](int *ptr){

This is still a copy of the pointer. It will work if I use a reference though:

auto doStuffWith = [](int* &ptr){

Long run though, I should probably start using smart pointers:

    std::shared_ptr<int> ptra(new int);
    std::shared_ptr<int> ptrb(new int);

    for(auto ptr: { &ptra, &ptrb }){
        // do stuff
        ptr->reset();
    }
    Q_ASSERT(ptra == nullptr);

    // for qobject
    QPointer<QObject> ptrc = new QObject();
    QPointer<QObject> ptrd = new QObject();

    for(auto& ptr: { ptrc, ptrd }){ // working with copy of qpointer, but meh
        // do stuff
        delete ptr;
    }
    Q_ASSERT(ptrc == nullptr);

[Christian Ehrlicher]

@fcarney said in Make shift list to modify pointed to objects:

for(auto& ptr: { ptrc, ptrd }){ // working with copy of qpointer, but meh

Nothing is copied here...

[fcarney]

@Christian-Ehrlicher

{ ptrc, ptrd }

That was a crux of the original problem. This structure or whatever it is, has a copy of ptrc and ptrd objects.

When the objects were just pointers it was copies of the pointers. That is why setting to null only worked on the copy itself.

[fcarney]

Oh interesting. The braces with objects create an std::initializer_list. That is why it works as a container with range for loops.

[Kent-Dorfman]

I think this is an example when you should use the STL containers. I frequently create containers of pointers of objects (handles) and iterate over them, but unless the container contains smart pointers you must remember to explicitly delete the objects.