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.