18

Consider the following program:

#include <string>

struct S {
    S (){}

private:
    void *ptr = nullptr;
    std::string str = "";
};

int main(){}

This, when compiled with -Weffc++ on GCC 4.7.1, will spit out:

warning: 'struct S' has pointer data members [-Weffc++]
warning:   but does not override 'S(const S&)' [-Weffc++]
warning:   or 'operator=(const S&)' [-Weffc++]

That's no problem normally, except for a couple things with this example:

  1. If I comment out any of the constructor, the pointer declaration, or the string declaration, the warning disappears. This is odd because you'd think the pointer alone would be enough, but it isn't. Furthermore, changing the string declaration to an integer declaration causes it to disappear as well, so it only comes up when there's a string (or probably other choice classes) with it. Why does the warning disappear under these circumstances?

  2. Often times this warning comes up when all the pointer is doing is pointing to an existing variable (most often maintained by the OS). There's no new, and no delete. When the class with the handle, in these cases, is copied, I don't want a deep copy. I want both handles to point to the same internal object (like a window, for example). Is there any way to make the compiler realize this without unnecessarily overloading the copy constructor and assignment operator, or disabling the warning completely with #pragma? Why am I being bothered in the first place when the Rule of Three doesn't even apply?

Robᵩ
  • 163,533
  • 20
  • 239
  • 308
chris
  • 60,560
  • 13
  • 143
  • 205
  • 1
    When I first saw this warning, I thought it meant "F*ck C++", not "Effective C++". They need to choose better names for their warnings. – S.S. Anne Aug 04 '19 at 03:35

3 Answers3

27

GCC's -Weffc++ has several issues, I never use it. The code that checks for "problems" is pretty simplistic and so the warnings end up being far too blunt and unhelpful.

That particular warning is based on Item 11 of the first edition of Effective C++ and Scott changed it (for the better) in later editions. The G++ code doesn't check for actual dynamic allocation, just the presence of pointer members.

See what I wrote about this warning in GCC's bugzilla when comparing the guidelines in the first edition with the third edition:

Item 11: Define a copy constructor and an assignment operator for classes with dynamically allocated memory.

Replaced by Item 14: "Think carefully about copying behavior in resource-managing classes" - the advice is less specific, but more useful. I'm not sure how to turn it into a warning though!

Jonathan Wakely
  • 166,810
  • 27
  • 341
  • 521
  • 3
    Another problem with -Weffc++: Having a non-virtual destructor on a *private* base class is not allowed, for no good reason. – Johan Lundberg Feb 19 '13 at 20:06
5

  1. When you do it, you have a POD structure. As it cannot have any constructors, -Weffc++ doesn't bother checking.

  2. Use a reference or shared_ptr object or any other object that wrap a pointer.

Jonathan Wakely
  • 166,810
  • 27
  • 341
  • 521
Arpegius
  • 5,817
  • 38
  • 53
  • 1
    I kind of thought it might be something like that for #1. But for #2, I've never really seen or imagined the use of them for handles used to interact with the system. Passing the pointer around is all you do; you can treat it as if it's not a pointer at all, as all that's important is the address it points to, not what's in there. A window handle is technically a `void *`. The use goes something like `HWND hwnd = CreateWindow (...); ShowWindow (hwnd, SW_SHOW);` Every pointer aspect is taken care of already; it's sort of like it's already a smart pointer. What do you do in that case? – chris Jul 16 '12 at 04:04
  • Don't know whats `HWND` but if that's a pointer and you have `CreateWindow`, you probably have to call `DestroyWindow` os something. Then it's good place for shared_ptr with `DestroyWindow` binded to destructor, it not, `-Weffect` is not for you. – Arpegius Jul 16 '12 at 08:41
  • Yes, that's one of the many objects stored as a `void *`. The funny thing about this one is that `DestroyWindow` is usually what *makes* the window handle go out of scope. Others might not require you to call something to delete them. The thing about `shared_ptr` is that it's like a `HWND *`, which is unnecessary. It should be more like a `shared_ptr`, but the handles aren't something you dereference or anything. In this case, the pointers can be thought of as normal variables. – chris Jul 16 '12 at 08:53
  • Yeah it looks like HWND is not design for C++. Anyway is alwas good idea to wrap old C structures into some classes, to make C++ more effective. The protip is that the class plays role in conceptual modeling. – Arpegius Jul 16 '12 at 13:58
  • Yeah, most of the winapi is C. The problem is when you wrap them in classes, but since they are technically pointers, the warning kicks in uninvited. – chris Jul 16 '12 at 14:11
  • So you realy should consider overloading copy constructor and operator= in your wrapper, even if you just need to make them private, without any implementation, and then keep pointers to wrapers by shared_ptr. Anyway is not that I don't know aobut winapi, is more like I don't want to know, so technicly I was trolling. – Arpegius Jul 16 '12 at 15:08
  • Well, that's depressing. Guess it just boils down to those options. – chris Jul 16 '12 at 15:14
  • Well, bad designed code can be even more depresing. That's what i'm thinking about WinApi. Anyway if you don't want to code too much then templates are your friends. – Arpegius Jul 16 '12 at 15:48
0

Old question but "That's no problem normally" is not totally true:

-Weffc++ force your code to reflect the behavior. What is saying here is if you are using the code as it is, the implicit copy operator will just make "ptr" point on the same address as the original struct.

MAYBE THAT WHY YOU WANT, but it just warns you because it is an implicit behavior. This can be clear for you that are writing the code, but will it be clear for an other dev?

What you should do to get rid of this warning is either explicitly delete the copy and = operator or define them. To delete a copy constructor you can do as follow:

S(const S&) = delete;
digitalTrilunaire
  • 97
  • 1
  • 13