Make shift list to modify pointed to objects
-
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?
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 notif(*ptr), which checks if the value pointed to is non-zero. -
-
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@fcarney Hah, right, I took too fast of a glance. Well, there's your answer. It doesn't help readability ;)
-
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 Hah, right, I took too fast of a glance. Well, there's your answer. It doesn't help readability ;)
@Chris-Kawa I changed the code to be explicit ** in my code. auto is too confusing.
-
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.
Work on the reference:
@fcarney said in Make shift list to modify pointed to objects:
for(auto &ptr: {ptr1, ptr2}){ -
@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.
C++ is a perfectly valid school of magic.
-
@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.
@fcarney That's 0:2 in the game of readability vs this code :)
-
@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.
@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);
Be aware of the Qt Code of Conduct, when posting : https://forum.qt.io/topic/113070/qt-code-of-conduct
Q: What's that?
A: It's blue light.
Q: What does it do?
A: It turns blue. -
@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);@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);C++ is a perfectly valid school of magic.
-
@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);@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...
Qt Online Installer direct download: https://download.qt.io/official_releases/online_installers/
Visit the Qt Academy at https://academy.qt.io/catalog -
@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...
{ 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.
-
Oh interesting. The braces with objects create an std::initializer_list. That is why it works as a container with range for loops.
-
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.
