Posts Tagged ‘unsolicited advice

22
May
13

Default assignment operators and const values

It’s been so long! Update: I’ve been working primarily in C++ for the past four months or so. Hooray! I have so many drafts. I’ve really got to just start posting stuff even when it’s not (gasp) perfectly complete. Today, we’ve got another thing that I didn’t actually learn today, and it’s pretty basic, but I encountered a surprisingly bad suggestion from Microsoft’s documentation, and it seemed worth commenting on. You C++ gurus can probably skip this one.

You may recall, lo these many months ago, I posted about warnings-as-errors and why you should enable it. Well, I’m in the position of taking a large existing project and trying to turn that functionality on, and let me tell you, even with a lot of the work done for me — I was not the first, or even the second, engineer to tackle this — it’s no picnic. Reminder: turn on warnings-as-errors from the start of your project.

Anyway, one of the issues that has come up is the following:

c4512: The compiler cannot generate an assignment operator for the given class. No assignment operator was created.

For those of you not in the know, the C++ compiler generates a default copy constructor and assignment operator for you, unless you’ve specified them yourself. This warning simply means that, for some reason, the default assignment operator cannot be generated. Note that if some part of the code were actually attempting to use this operator, the compiler would throw an error, rather than a warning. Microsoft’s documentation on the warning helpfully points out that if you have any const members in your class, you will get this warning because a const member is not modifiable and thus assignment to it will not work. It continues to add that your options for dealing with the warning are the following:

  • Explicitly define an assignment operator for the class.

  • Remove const or the reference operator from the data item in the class.

  • Use the #pragma warning statement to suppress the warning.

The engineer who looked at this before me went for both the first and the second option. The second option is the wrong option, and frankly, Microsoft should be ashamed for even suggesting it. And while the third option is appropriate for some warnings, in this case, completely ignoring the error is almost certainly never the right answer. Even the first option isn’t quite on target, or at least ignores a possible solution.

I haven’t talked about const much in this blog, but I’m a huge fan. If a member variable is declared const, any programmer using that API can assume that once the data is set in the constructor, it will not change. Removing const-ness should not be done lightly, especially when (as in this case) the member is public. Removing this type modifier opens up that member to modification by any other code. Scott Meyers would tell you to “use const whenever possible” because it is such a strong contract, and I’m inclined to agree with him.

As I mentioned before, if any code were actually attempting to use the assignment operator, you would get an actual error — the code could not be generated, so anything attempting to use that code would not compile. So we don’t need an assignment operator. We just need for the compiler not to generate one. The fix for this is easy — declare it private in the header file and don’t define it. You can do this yourself (there are a million examples on the internet), or you can just make your class inherit from boost::noncopyable, which will also declare the copy constructor private, if you happen to be using the Boost libraries already.

This is a really great practice even if you’re not getting this warning. (I believe it’s one of the first item’s in Effective C++.) If you don’t expect your class to be copied, go ahead and declare the copy constructor and the assignment operator private.

26
Jul
11

Yes (/WX)

About a month ago, I realized that I was learning a lot and not writing it down. So I decided to start writing it down. A month later, I don’t remember what I learned that was so important, but I figured I should just start writing. Some of it will be technical. Some of it will be common sense. Most of it, you’ll probably have heard before somewhere. I’ll start with something that I tend to re-learn every project.

Treat warnings as errors: Yes (/WX)

I know some of you are saying, “But I plan to use that unused variable later!” or “Hey, I *know* that variable gets initialized in one of the branches of my if case, so it’s okay.” To put it bluntly, you’re wrong.

If you’re not using the variable now, replace it with a TODO comment, which has at least some chance of reminding you that you were supposed to do something with it. (No, it’s not a good reminder, but trust me, one more warning in the inevitable sea of warnings will not be any better.) And as for that variable that you know is going to be initialized in one of those branches, when was the last time you touched a piece of code and it never changed ever? That never happened? That’s what I thought.

But why should you care that your cup overfloweth with warnings every time you build your solution? Well, for one thing, they’re warnings, not beer. They’re an indication that something is behaving not quite as intended. It may mean a little bit of code bloat, which is not such a big deal. Or it could mean that you wrote this code when you were really tired, and you didn’t actually use the variable you thought you were using. But if it’s only a warning, amid a sea of warnings, you’re not going to notice until your code doesn’t do what you thought it should do, and it’s the middle of crunch and you’ve just forced QA to fail the build that was supposed to go to cert.

Or, more importantly, you might find that some other project that you didn’t build locally does treat warnings as errors and then your change fails on the incremental build, and everyone will point at you and laugh.




About

My name is Maitland Lederer, and I’m a video game developer. I learn stuff you probably already knew and have opinions you've probably already heard. I figured it might be a good idea for me to start writing down the stuff I've learned so I don't have to relearn it. It's not, like, great wisdom or anything. It's just things I happened to learn, usually today.

Header photo by D Sharon Pruitt, used under a Creative Commons License.