2

Say I have an array std::vector<Foo>, and I want to iterate over all my foos and do stuff do them like so:

for (auto foo : vecFoo)
    foo.x = 10;

This ends up doing nothing because it's making a local copy of the contents of vecFoo instead of a reference to it. The correct loop is as follows:

for (auto& foo : vecFoo)
    foo.x = 10;

This is a mistake I've made a few times now, so I'd like to find a solution for it that will catch me when I get it wrong. I would be happy with either something I can do to the struct or a warning flag I can turn on. I've tried making Foo's copy constructor private, but then I end up being unable do push_back or emplace_back, which is clearly not what I want.

18
  • 16
    Experience and muscle memory Sep 4, 2012 at 16:12
  • 4
    @DavidRodríguez-dribeas - and unit tests. Sep 4, 2012 at 16:15
  • 3
    For you example - use std::fill. Problem gone! :-)
    – jrok
    Sep 4, 2012 at 16:18
  • 1
    emplace_back shouldn't use a copy constructor, it should use the move constructor. Sep 4, 2012 at 16:19
  • 4
    @Alex: The problem is that whether you want a reference or a copy depends on usage, and both are valid in the language. You cannot tell the compiler: please ignore that this is valid and generate an error because I tend to forget. Again, the most important thing is to learn to add the & Sep 4, 2012 at 16:19

3 Answers 3

4

The answer to the underlying problem is learning. If you make the mistake a couple more times you will end up learning from experience.

As of language tricks for your particular case, where you have full control over the type that is stored in the container, you can provide a nothrow move constructor and disable copy construction. That will require the use of emplace_back (or move semantics with push_back).

Still, this is only a way of triggering an error in this particular case, but if you store any type that is not 100% under your control (i.e. cannot disable copy construction or add move construction, or cannot implement a nothrow move constructor --cannot guarantee that it will not throw) then you are out of luck, which leads to the first sentence: learn to use references

3
  • @Xeo: That is an interesting idea. I would not do it, but it is nonetheless an interesting idea. Note that the wrapper must still offer the same properties: no copy constructor (easy) and nothrow move constructor. The second is a bit trickier. Consider that the copy constructor of the wrapped type can throw, then you cannot provide a wrapper that implements a heavy move by copying the internal object, so you would have to wrap a pointer (which is trivial to offer the nothrow move constructor). Then the memory layout of the application changes and you'll need to update the code... Sep 4, 2012 at 19:07
  • ... and you end up breaking the design just because you don't want to have to remember whether you want to iterate over a container copying or holding references. Do you really want to go through this refactor just not to have to remember to write an ampersand? I don't. Sep 4, 2012 at 19:08
  • Obviously, I'd write an ampersand over this convoluted workaround any day, but I like strange ideas. :)
    – Xeo
    Sep 4, 2012 at 19:19
1

This is a mistake I've made a few times now, so I'd like to find a solution for it that will catch me when I get it wrong.

In my experience, the best solution is to train yourself to always declare objects as const by default. This makes it a compiler-checked error to try to modify them. Once you do that, the compiler will catch such a declaration for you when you have forgotten to deviate from it to make an object writeable:

// This is what my hands write by default:
int const x = 42;

I try to always write like this. Then, whenever I see the following in code:

int y =  42;

My mind tries to figure out if the variable should actually be modifiable. You can adapt this thinking to your loop example because thankfully, C++11 allows this for range-for loops as well:

for (auto const i : vecFoo)
    i.foo = 10; // error

Of course this only works once you’ve trained yourself to automatically const everything.

I agree that this is a far cry from perfect and requires considerable exercise. As several people have repeatedly noted, the best solution would be for C++ to consider declarations const by default (and in fact at least for lambda-captured values this is the case) but that ship has sailed.

7
  • 3
    I think he wants the i.foo = 10 to work, but it's not working because he's making a copy of the object with auto instead of a reference with auto&. Sep 4, 2012 at 16:17
  • @Seth Sure. But once you train yourself to always write const by default, the compiler will subsequently complain if you have forgotten to make an object a (mutable) reference instead of a const). This requires a considerable bit of mental training to get into the habit of consting everything but I don’t know a better way. See update. Hopefully this makes it clearer. And yes, it’s not a particularly good solution. Sep 4, 2012 at 16:21
  • 1
    Should be: static const unsigned int x = 42; :) Sep 4, 2012 at 16:29
  • 1
    @Alex I haven’t misread it. And I agree that the solution is rotten but it works (and it has the benefit of making the code more explicit and thus more amenable to automated error checking): train yourself to always declare variables const. Then, if you want to modify a variable, you make a mental effort to get the declaration right because otherwise the compiler complains. Sep 4, 2012 at 16:38
  • 1
    @Mustafa No, why should it? The 42 is just an example. It’s a general pattern, I don’t only do it for compile-time constants. The whole point is doing it consistently everywhere. Sep 4, 2012 at 16:39
1

What I am asking for is a way to make that copy not valid for my class so that the opportunity to make such a mistake is not made.

C++11 makes it easy to make your class uncopyable. Just delete the copy constructor/assignment operator (assuming your compiler supports that C++11 feature):

class SomeClass
{
public:
  SomeClass(const SomeClass &) = delete;
  SomeClass &operator=(const SomeClass &) = delete;

...
};

Any attempt to call the copy constructor will yield a compiler error. If you're using Visual Studio, which doesn't yet support this syntax, you'll have to use the standard C++03 idioms for this. Namely, declaring the copy constructor privately.


I can no longer use my struct in vectors since it can not be copied into the vector.

Sure you can. You just can't use functions that require copying. You have to replace your uses of push_back and insert with emplace_back and emplace.

Remember: if you declare a copy constructor and assignment operator, even just to delete them, you must manually declare a move constructor/assignment operator. Of course, you can use = default syntax.

3
  • This appears to fail for the same reason making the copy constructor private fails as mentioned in the question: I can no longer use my struct in vectors since it can not be copied into the vector.
    – Alex
    Sep 4, 2012 at 17:43
  • I must be doing something wrong then (or there is a bug in g++) because I can not get it to work. This does not compile for me. I get these errors.
    – Alex
    Sep 4, 2012 at 20:11
  • Thank you, that was the piece of the puzzle that I was missing.
    – Alex
    Sep 4, 2012 at 20:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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