I had a bug in my OpenGL program. Here was the original code:
for (Orbitor o : orbitors){ o.calculate_position(); } |
and here was the working version
for (std::vector<Orbitor>::iterator it = orbitors.begin() ; it != orbitors.end(); ++it) { it->calculate_position(); } |
The bug was that the o.calculate_position(); call was supposed to update the internal state of the Orbitor structure, but was called on a copy of the instance in the original structure, and not on the original structure itself. Thus, when a later call tried to show the position, it was working with the version that had not updated the position first, and thus was showing the orbitors in the wrong position.
The reason for this bug makes sense to me: the first version of the loop makes a copy of the instance to use inside the for loop. The second uses a pointer to the original object.
One way that I can protect against these kinds of bugs is to create a private copy constructor, so that the objects cannot be accidentally copied.
Would this work better for you? One less dereference.
for (auto& o : orbitors){
o.calculate_position();
}
Couldn’t this also have been fixed by using
for(Orbitor &o : orbitors){
o.calculate_position();
}
That is, explicitly make the local for variable a reference to the object you want to manipulate ?
You could also use a reference in the range based for loop. It’s easier to read.
If Orbitors should never be copied, you could also delete the copy constructor and copy assignment operator. I’ve never thought about making them private. Thanks for that idea. The C++ reference encourages to set both functions private or to delete them.
https://en.cppreference.com/w/cpp/language/rule_of_three
Thanks to all three of you for showing me the error of my ways! Yeah, that makes much more sense.
It has been a long time since I worked heads down in C++ and I’m still remembering…plus learning those things that have become standard in the past decade.
I’m still not certain if I want a copy constructor or not. I think not, but I am not certain, so I will hold off for the moment.
The orbitor is pretty much a data transfer object with only the slightest bit of behavior that I suspect could be externalized, allowing one to customize how an orbitor’s position is calculalted or displayed. There is, thus far, no need to clone it. I guess I could make private for now and always expose them if I do need.
Extra points for the Tagon avatar.
A C++20 option is std::ranges::for_each(orbitors, &Orbitor::calculate_position)
Before c++20 you’d need to std::for_each(orbitors.begin(), orbitors.end(), &Orbitor::calculate_position)
There’s no particular advantage to this more functional style vs the range-for loop, but IMHO in this particular case the brevity matches the simplicity of what you’re doing.