3

I'm looking through some legacy code and I've come across a case where the developer has 'extended' an existing class with additional members, using inheritance as his weapon of choice. Essentially it reduces to something like this:

struct A
{
    int something_old;
};

struct B : public A
{
    int something_new;
};

There's a section of code where they have an instance of A, but actually want an instance of B. Here's how they managed to achieve it:

auto aa = GetAn_A(); // for example
   :
B bb;
*(A*)&bb = aa; // <-- code smell?
bb.something_new = 42;

This seems to work ok: it slices bb and then assigns to the sliced part. But I can't help but thinking that in these enlightened C++ days, that all of that hacking 'n' slicing with &'s and *'s is a bit old school.

Is there a code smell here and, if so, how big of a smell? Should could like this still need to be written?

If it is smelly, where did the author start to lose the plot, and what could/should they have done to get the result they wanted, i.e. an extension to a existing class (bearing in mind that A and B are significantly more complicated than this in reality, and that A was written some time before B)?

  • @Deduplicator: alas, it doesn't work (at least not in MSVC): no acceptable conversion. – WalderFrey Nov 24 '17 at 15:23
  • 1
    I don't like acting like you can take an A when you need a B. Putting Bs where As are expected is ok. Not the other way round. Also I don't like mixing the code that finds the B with code that uses the B. It's better to let the B be handed to you. – candied_orange Nov 24 '17 at 15:36
  • In the actual code, do these types have constructors, such as these examples? – rwong Nov 24 '17 at 16:19
  • According to this answer and re-wording of the C++ standard, your "code smell" could trigger UB. – rwong Nov 24 '17 at 16:22
  • 1
    @rwong: That paper is not in play here, since he is not creating a new object. He's not doing new((A*)&bb) A(...). He's assigning to an existing object. – Nicol Bolas Nov 24 '17 at 16:27
8

It all depends on what A and B are and mean.

Because base class subobjects can do things that member subobjects cannot (being empty not disturbing the layout of the type, automatically exposing their member functions as the owners, etc), there are times when inheriting from a class makes sense even without polymorphism.

In the above case, neither type is polymorphic, so inheritance is being used for non-dynamic polymorphic purposes. That is, the user's intent is not to treat B as if it were an A.

Essentially, the user is treating the base class of B like a member subobject. There's a function that returns a value of type A, and you want to store it in an object of type B. The fact that it's a base class rather than a member is essentially an implementation detail.

Note that C++17 normalizes this interpretation to a degree. In your example, B is an aggregate in C++17 and can use aggregate initialization to initialize all of its subobjects:

B bb = {aa, 42};

This kind of thing can be quite useful. The ability to have the methods of the base class be members of your class is something you cannot do easily if you make it a member. For example, if you're making a string_view-like class with some additional members, it'd be silly to write dozens of forwarding functions when inheriting from it when you can just derive from it. While public inheritance is a little overboard, not everyone wants to write a bunch of using declarations.

Inheritance, even public inheritance, does not always mean "is a".

Now, I'd still fail it at code review because of the way the assignment is handled. It just should be (A&)bb = aa;.

  • Thanks Nicol. Your answer looks to be bang on the money. I believe that the developer simply wanted to 'add' a few member variables to an existing struct, and used inheritance exactly as you suggest to expose the base class's members. Your improved syntax for the assignment is much appreciated. – WalderFrey Nov 24 '17 at 16:44
0

In addition to @NicolBolas excellent answer; we can do that sequence using constructors and without casting.

...
A aa = GetA();
...
B bb(aa);    // instead of: B bb; *(A*)&bb = aa;
...

For this, A and B need appropriate constructors (and note that those constructors also won't require awkward casts).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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