views:

1052

answers:

9

Hello, Gurus,

I want to know if it is possible to let compiler issue a warning/error for code as following:

Note:

1. Yea, it is bad programming style and we should avoid such cases - but we are dealing with legacy code and hope compiler can help identify such cases for us.)

2. I prefer a compiler option (VC++) to disable or enable object slicing, if there is any.

class Base{};
class Derived: public Base{};

void Func(Base)
{

}

//void Func(Derived)
//{
//
//}

//main
Func(Derived());

Here if I comment out the second function, the first function would be called - and the compiler (both VC++ and Gcc) feels comfortable with that.

Is it C++ standard? and can I ask compiler (VC++) to give me a warning when met such code?

Thanks so much!!!

Edit:

Thanks all so much for your help!

I can't find a compiler option to give a error/warning - I even posted this in MSDN forum for VC++ compiler consultant with no answer. So I am afraid neither gcc nor vc++ implemented this feature.

So add constructor which take derived classes as paramter would be the best solution for now.

Edit

I have submit a feedbak to MS and hope they will fix it soon:

https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=421579

-Baiyan

+1  A: 

This is commonly called Object Slicing and is a well-known enough problem to have its own Wikipedia article (although it is only a short description of the problem).

I believe I have used a compiler that had a warning you could enable to detect and warn about this. However, I don't recall which one that was.

Greg Hewgill
+1  A: 

Not really a solution to your immediate problem, but....

Most functions that take class/struct objects as parameters should declare the parameters to be of type "const X&" or "X&", unless they have a very good reason not to.

If you always do this, object slicing will never be a problem (references don't get sliced!).

Drew Hall
Although it's good advice, I have to -1 this since we now know that the asker is dealing with a pile of legacy code, so design advice is unhelpful.
j_random_hacker
While this advice is, in general, good, in the presence of a slicable object hierarchy it can defer the slicing to a place where it is impossible to detect at compile time.
Charles Bailey
@j_random_hacker: Didn't know that when I answered. You're correct that it's not helpful to him now, but I'll keep the answer since it might help somebody avoid the problem in the future.
Drew Hall
@Charles Bailey: Can you elaborate?
Drew Hall
@Drew: Yes it's a pain when the asker leaves out some critical tidbit and then adds it after you answer... Re: Charles' comment, see his answer. In short, even with a full recompile you can't detect slicing if a func in a separate source file takes a reference param and then treats it as a value.
j_random_hacker
+1  A: 
class Derived: public Base{};

You're saying Derived IS a Base, and so it should work in any function that takes a base. If this is a real problem, maybe inheritance isn't what you really want to be using.

BigSandwich
In C++ with pass-by-value semantics, this is a real problem because virtual functions on Base called from within Func may not have the complete Derived object to work with.
Greg Hewgill
Yes, you already brought that up, but that's not his only problem. He could have completely different logic in each of those functions, and it doesn't seem like he fully understands inheritance semantics.
BigSandwich
That's quite possible. :)
Greg Hewgill
We are dealing with legacy code, and need to monitor such cases when refactoring. Yea, it is not good programming style, and my focus is: Is there a way to let compiler generate a warning?
lz_prgmr
Have to -1 this since we now know that the asker is dealing with a pile of legacy code, so design advice is unhelpful.
j_random_hacker
+4  A: 

I would suggest adding a constructor to your base class which takes a const reference to the derived class explicitly (with a forward declaration). In my simple test app, this constructor gets called in the slicing case. You could then at least get a run-time assertion, and you could probably get a compile-time assertion with clever use of templates (eg: instantiate a template in a way which generates a compile-time assertion in that constructor). There may also be compiler-specific ways to get compile time warnings or errors when you call explicit functions; for example, you can use "__declspec(deprecated)" for the "slice constructor" in Visual Studio to get a compile-time warning, at least in the function-call case.

So in your example, the code would look like this (for Visual Studio):

class Base { ...
    __declspec(deprecated) Base( const Derived& oOther )
    {
        // Static assert here if possible...
    }
...

This works in my test (compile-time warning). Note that it doesn't solve the copy case, but a similarly-constructed assignment operator should do the trick there.

Hope this helps. :)

Nick
Thanks Nick, this is really cool!But in my case, I even don't know which classes has such problem - I need compiler to help me find such cases, in tons of code.-Baiyan
lz_prgmr
+10  A: 

If you can modify the base class you could do something like:

class Base
{
public:
// not implemented will cause a link error
    Base(const Derived &d);
    const Base &operator=(const Derived &rhs);
};

Depending on your compiler that should get you the translation unit, and maybe the function where the slicing is happening.

Andrew Khosravian
Even better is to make the unimplemented copy constructor and assignment operator private. And this assumes you don't actually need either.
bk1e
+1, very nice solution. (And might I point out that this is the first *actual solution* to the question asked -- all the other "solutions" have simply explained why it's a bad idea to combine by-value semantics with inheritance.)
j_random_hacker
Yea, it is similar to Nick's solution, it is cool, but it assumes we already know which class has such problems - it is impractical since there are so many classes in our code base.But thanks, it really helps to idenftify the object slicing for known classes.
lz_prgmr
@Baiyan: You can reduce the number of keystrokes by #defining a macro DISALLOW_SLICE(base, derived). Also, if your codebase is consistently formatted, you could write a script that inserts this code just before the end of each class. It's a hack, but it could save you some time.
j_random_hacker
+1  A: 

The best way to combat this problem is usually to follow Scott Meyer's recommendation (see Effective C++) of only having concrete classes at the leaf nodes of your inheritance tree and ensuring that non-leaf classes are abstract by having at least one pure virtual function (the destructor, if nothing else).

It is surprising how often this approach helps clarify the design in other ways, as well. The effort of isolating a common abstract interface is usually a worthwhile design effort in any case.

Edit

Although I originally didn't make this clear, my answer comes from the fact that it is not possible to warn accurately about object slicing at compile time and for this reason it can lead to a false sense of security if you have a compile time assertion, or a compiler warning enabled. If you need to find out about instances of object slicing and need to correct them then it implies that you have the desire and ability to change the legacy code. If this is the case, then I believe that you should seriously consider refactoring the class hierarchy as a way of making the code more robust.

My reasoning is this.

Consider some library code that defines a class Concrete1 and uses it in the inferface to this function.

void do_something( const Concrete1& c );

Passing the type be reference is for efficiency and is, in general, a good idea. If the library considers Concrete1 to be a value type the implementation may decided to make a copy of the input parameter.

void do_something( const Concrete1& c )
{
    // ...
    some_storage.push_back( c );
    // ...
}

If the object type of the passed reference is, indeed, Concrete1 and not some other derived type then this code is fine, no slicing is performed. A general warning on this push_back function invocation might produce only false positives and would most likely be unhelpful.

Consider some client code that derives Concrete2 from Concrete1 and passes it into another function.

void do_something_else( const Concrete1& c );

Because the parameter is taken by reference no slicing occurs here on the parameter to check, so it would not be correct to warn here of slicing as it may be that no slicing occurs. Passing in a derived type to a function that takes a reference or pointer is a common and useful way to take advantage of polymorphic types so warning or disallowing this would seem counter-productive.

So where is there error? Well the 'mistake' is passing in a reference to something that is derived from a class that is then treated as though it is a value type by the called function.

There is, in general, no way to generate a consistently useful compile time warning against object slicing and this is why the best defence, where possible, is to eliminate the problem by design.

Charles Bailey
Did you read the question? He already has a bunch of legacy code and wants to find where slicing is occurring. What you're saying is all good advice for someone designing a new system, but it doesn't help this guy.
j_random_hacker
It's legacy code but he's obviously having to modify it. Modifying class hierarchies is definitely an action that should be considered when trying to improve legacy code.
Charles Bailey
I disagree that he "obviously has to" modify it. Perhaps there are just 1 or 2 bugs in this million-line codebase that he needs to fix. Modifying class hierarchies in legacy code is something you want to consider *as a last resort*, since it's one of the most labour-intensive things you could do.
j_random_hacker
Well, I was just assuming that the code needs changing. If it works as is, then strictly pragmatically, there's no need to worry if slicing happens. If the one or two bugs need to be fixed then surely it's time to look at a minimal risk approach to getting them fixed?
Charles Bailey
Sure, if you have infinite time and resources, that is the right way to do it. But usually we can't afford "minimal risk", we can only afford "minimal time+risk" with a large weighting on time. And BTW, completely redesigning a large class hierarchy is not a risk-free undertaking.
j_random_hacker
The comments are a bit short for this, so I've expanded on my answer.
Charles Bailey
You make a good point about deferring object slicing. To detect cases like this, you would need to recompile everything, which of course may not be possible. The fact that this situation is possible (and undetectable) is a bit of a design bug in C++ itself methinks!
j_random_hacker
Yes, and even if you recompile everything it would still require a compiler to look at all translation units at once to find the possibility that some deferred slicing might take place, not a good task to be undertaking for every source file. It's not possible to detect in a basic static analysis.
Charles Bailey
You're right, deferred slicing is undetectable even with a full recompile, if the code that takes the reference parameter is in a different translation unit (as would commonly be the case for a library function). Ouch!
j_random_hacker
+2  A: 

Seems you guys are more interested in "How to design classes" rather than the real problem.

IMHO, everyone knows how to design classes...

lz_prgmr
+4  A: 

As a variation of Andrew Khosravian's answer, I'll suggest using templated copy constructors and assignment operators. This way you don't need to know all the derived classes of a given base class in order to safeguard that base class against slicing:

class Base
{
private:   // To force a compile error for non-friends (thanks bk1e)
// Not implemented, so will cause a link error for friends
    template<typename T> Base(T const& d);
    template<typename T> Base const& operator=(T const& rhs);

public:
// You now need to provide a copy ctor and assignment operator for Base
    Base(Base const& d) { /* Initialise *this from d */ }
    Base const& operator=(Base const& rhs) { /* Copy d to *this */ }
};

Although this reduces the amount of work needed, with this approach you still need to mess with each base class in order to safeguard it. Also, it will cause problems if there are legitimate conversions from Base to SomeOtherClass that employ an operator Base() member of SomeOtherClass. (In that case, a more elaborate solution involving boost::disable_if<is_same<T, SomeOtherClass> > can be used.) In any case, you should remove this code once you've identified all instances of object slicing.

To the compiler implementors of the world: Testing for object slicing is definitely something that would be worthwhile creating (optional) warnings for! I can't think of one instance where it would be desired behaviour, and it's very commonly seen in newbie C++ code.

j_random_hacker
haha, we think alike. i was going to give him teh same stuff :p combined with declspec deprecated / __attribute__ deprecated, this one would rock i think :p i would also not care about operator Base() in a derived. that's hell ugly anyway :p but +2 for u indeed
Johannes Schaub - litb
you can however put enable_if in the two so they are only enabled for derived classes. so conversions for an unrelated class to Base would still work (boost::is_base_of i think)
Johannes Schaub - litb
Thanks litb. Though I was meaning to allow a legitimate operator Base() in a non-derived class. I think you need to *disable* conversions for derived classes since those are the conversions you are trying to detect!
j_random_hacker
Whoops, I got what you were trying to say round the wrong way. I think we're saying the same thing -- you're suggesting enabling the templates for derived classes only, I'm suggesting disabling for legitimate non-derived classes. :)
j_random_hacker
A: 
Intriguing idea, but this also forbids "Func(Base());" on both MinGW (g++ 3.4.5) and Comeau. That is, you can't actually pass a Base object as a value either! OTOH, MSVC++9 allows both "Func(Base());" and "Func(Derived());". Would be interesting to know what the standard says about this.
j_random_hacker
Actually it seems like a weird rule of C++ means that, in some cases, even for classes passed by reference, the copy ctor must be accessible. See here: http://gcc.gnu.org/bugs.html#cxx_rvalbind
j_random_hacker