10
\$\begingroup\$

I have a number of related classes (Burger, Pizza, Cupcake) with a common base class Food and a function eat which consumes class instances. The design shown here works, but I can't help but feel it could be made better or more elegant. Can you help me make a more nutritious and satisfying lunch?

First, here's the code:

lunch.cpp

#include <iostream>
#include <string_view>
#include <vector>
#include <memory>

class Food {
public:
    Food(const std::string_view &name) : myname{name} {
        std::cout << "Creating " << myname << "\n";
    }
    virtual std::string_view name() const {
        return myname;
    }
    virtual ~Food() {
        std::cout << "the " << myname << " is gone\n";
    }
private:
    const std::string_view myname;
};

#define FOODTYPE(x) \
class x : public Food { \
public: \
    x() : Food{#x} {} \
    virtual std::string_view name() const override { \
        return "Overridden"#x; \
    } \
}

FOODTYPE(Burger);
FOODTYPE(Pizza);
FOODTYPE(Cupcake);

void eat(Food && food) {
    std::cout << "Just ate " << food.name() << '\n';
}

int main() {
    std::vector<std::unique_ptr<Food>> lunch;
    lunch.emplace_back(std::make_unique<Burger>());
    lunch.emplace_back(std::make_unique<Cupcake>());
    lunch.emplace_back(std::make_unique<Pizza>());
    lunch.emplace_back(std::make_unique<Cupcake>());
    std::cout << "Lunch is now ready\n";
    for (auto food{lunch.begin()}; food != lunch.end() && *food; ++food) {
        auto temp = std::move(*food);
        eat(std::move(*temp));
    }
}

Here are my questions:

Can I call a pure virtual function from a destructor?

OK, this is somewhat rhetorical, because the answer is clearly "no, you can't" but here's what I wanted to do:

class Food {
public:
    virtual std::string_view name() const = 0;
    virtual ~Food() {
        std::cout << "the " << name() << " is gone\n";
    }
};

class Pizza : public Food {
public:
    virtual std::string_view name() const override {
        return "pizza";
    }
};

Obviously, that won't compile because the Food destructor is attempting to call the pure virtual name() function. However, because it's a pure virtual function we know that a derived class implementation must exist (or we couldn't be calling the destructor) it seems like there ought to be a way to call that derived implementation from the base class destructor. Or is there a more elegant way to address this than what I've already written? Because I'm using C++17 and std::string_view, it seems like there ought to be some kind of constexpr way to do this. Maybe a template instead of a base class?

Can I use a range-for in the main() loop?

I had wanted to write something like this:

for (auto &food : lunch) {
    eat(std::move(food));
}

But that doesn't quite work as intended. The problem is that it leaves all of the Food still within Lunch so one could, in effect, eat the same food twice. Obviously that's not right, so that version is not correct. However, I think there may be a simpler way to write that loop.

Is there a way to use an initializer_list to create the vector?

What I'd have preferred would be to write something like this:

std::vector<std::unique_ptr<Food>> lunch{Burger(), Cupcake(), Pizza(), Cupcake()};

Of course that doesn't work, but something smaller and more succinct would be nice.

Yes, macros are ugly

However, they're also handy on occasion. If you hate the FOODTYPE macro, what would you have used instead?


Output

Here's the output from this program.

Creating Burger
Creating Cupcake
Creating Pizza
Creating Cupcake
Lunch is now ready
Just ate OverriddenBurger
the Burger is gone
Just ate OverriddenCupcake
the Cupcake is gone
Just ate OverriddenPizza
the Pizza is gone
Just ate OverriddenCupcake
the Cupcake is gone

Further details

The application for all of this is within a much larger program and in that context, there are producers of messages of different classes (represented here as different kinds of Food) and consumers of messages. In that larger software, things are multithreaded and use a thread-safe queue to handle passing messages. However, I wanted to get some concrete feedback on this part of the code first. Depending on the answers received from this question, I may post a follow-up which also contains some of the multi-threading elements, but didn't want to overcomplicate the initial question and it will take some effort to extract the multithreaded version from the larger application.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Something concerning that immediately springs to mind is the high calorie intake, maybe add some veggies to balance it out \$\endgroup\$
    – yuri
    Jun 16, 2018 at 19:51
  • 2
    \$\begingroup\$ Since it's a fully object-oriented Burger, the fact that it's actually made of black beans is hidden as a class implementation detail. \$\endgroup\$
    – Edward
    Jun 16, 2018 at 19:56
  • \$\begingroup\$ @Edward, since inheritance is used, I guess recompiling often is not an option? How much ABI stability is needed? Also it seems like objects are going to be long lived. \$\endgroup\$ Jun 16, 2018 at 20:22
  • \$\begingroup\$ @Incomputable: Objects may be long lived (e.g. a Cupcake probably lasts longer than a Burger before being consumed), but generally they will be consumed not long after creation. Recompiling often and ABI stability are not big concerns at the moment. \$\endgroup\$
    – Edward
    Jun 16, 2018 at 20:35

1 Answer 1

8
\$\begingroup\$

Let's start with the easiest of your questions:

Can I use a range-for in the main() loop?

Yes, you can.

for (auto& food : lunch) {
    if (food) {
        eat(std::move(*food));
        food.reset();
    }
}

does the trick nicely, and is quite a bit less ugly, in my opinion.


On to the next question.

Can I call a pure virtual function from a destructor?

As you correctly stated, no, you cannot. The reason for this is that, in the destructor, the this pointer is a pointer to an object of the destructor's class, and nothing more, which means that virtual calls to derived member functions are impossible. This also implies that it is impossible to retrieve such information at all from a derived class at the point of the base class' destruction, which is somewhat unfortunate.

Is there a good way around this? To be honest, I don't know of one. However, there is a pretty ugly trick you could exploit here: Since Food::name is currently only dependent on static data, you could turn it into a static method, and then lift the inheritance to a template level:

class FoodInterface {
    public:
    virtual std::string_view name() const = 0;
    virtual ~FoodInterface() {}
};

template <typename T>
class Food : public FoodInterface {
    public:
    virtual std::string_view name() const override = 0;
    virtual ~Food() override {
        std::cout << "the " << T::s_name() << " is gone\n";
    }
};

#define FOODTYPE(x) \
class x : public Food<x> { \
    public: \
    x() : Food{} { \
        std::cout << "Creating" #x "\n"; \
    } \
    static std::string_view s_name() { \
        return "Overridden"#x; \
    } \
    std::string_view name() const override { \
        return x::s_name(); \
    } \
}

(Or something like this; the above is just a quick sketch of an idea I had)

The introduction of another level of inheritance here has to do with the fact that turning Food into a template makes it impossible to use in a generic context, i.e. you can't make vectors of std::unique_ptr<Food> anymore, so you need another proxy class which can be used as a base, basically.

While this works, I consider it an ugly hack, as I mentioned before. Also, while this works with the contrived example you posted here, it might will easily collapse as soon as you cannot make the name statically available.

Another option to solve this, in this example at least, would be to just change the destructor you're focusing on, and have the destructor of the most derived class do the printing instead, which would be hidden behind the FOODTYPE macro anyway (although this quickly falls apart as soon as you have classes which aren't declared through this macro).


Next.

Is there a way to use an initializer_list to create the vector?

Sadly, no. There are basically two problems here:

  1. std::initializer_list is copy-only, so you can't really construct anything that's not copyable and
  2. the only feasible constructor of std::unique_ptr is explicit, thus blocking any attempts to construct from raw pointers.

Initially, I thought that something like std::vector<std::unique_ptr<Food>> lunch{new Burger(), new Cupcake(), new Pizza(), new Cupcake()}; might work, but sadly, problem 2 prevents this very effectively. Problem 1, on the other hand, prevents you from constructing unique_ptrs directly in the initializer list, since those cannot be copied.


Other observations

In void eat(Food && food), why did you opt to go with an rvalue reference? You're not moving from food, and you're not transferring ownership. This should just be void eat(Food const& food) instead.

Having the validity of a pointer as a break condition, as in for (auto food{lunch.begin()}; food != lunch.end() && *food; ++food), seems dubious to me. You're effectively asserting that once you find a nullptr, all pointers behind it are automatically invalid, which is not at all obvious from your code (in fact, the check for nullptr serves no real purpose in your code right now, since all your construction is so neatly contained. Again, I suppose this has deeper reasons not evident from your little mock snippet). If this is what you intended to express, please ignore this point (and adjust my code suggestion from the first answer accordingly).

Your use of std::string_view makes me deeply uneasy. While you are staying within safe boundaries here, std::string_view should be shunned as a class member as often as possible, since it suffers from the same lifetime implications that also affect references (if the underlying string dies, you have the equivalent of a dangling pointer/reference) and also express non-ownership, which, as far as I can see, is misleading here: Clearly, name/myname is owned by Food here; it is the only class that holds a reference to it. In general, this kind of situation calls for a different string class, one which expresses ownership, but doesn't incur the overhead of std::string. Sadly, such a class does not exist in the standard library; an easy semantic approximation could be gained through gsl::owner<std::string_view>, but what would really work well here, in my opinion, is some kind of copy-on-write string, but unless you go all the way and include a fitting library, or even write such a class yourself, it basically boils down to a "pick your poison"-situation.


Last, but not least, shall be the most substantial criticism I have to offer: Why do you even use inheritance here? The situation you present us does not call for it. Basically, you could achieve all this with a single class, Food, with a single member variable, name. All of this could be reduced to a basic, maybe 10-line program. Now, of course, I have to assume that you're aware of this, and that you are actually presenting us a strong abstraction over some piece of code you're working on, which is far more complex and justifies all of this inheritance design.

The problem that this entails is that, in order to answer your questions, a significant of guesswork is needed, so, for example, in the proposed solution from answer two. Of course, there are probably reasons why you don't want to or even can't disclose the real source code at all, but it would be nice of you to provide a more "real" program, with more bits of context, in the (possible) follow-up question, just to enable us to guess less and analyze more :)


Edit: Changed the first snippet to use std::move correctly. Before, the line eat(*food) would not compile, as eat takes an rvalue reference.

\$\endgroup\$
9
  • \$\begingroup\$ My compiler (gcc version 8.1) will not compile your first loop without explicit eat(std::move(*food)); but I agree that even with that, it's a bit nicer. Closely related is that the reason eat takes a move reference is exactly because it is intended to take ownership of the passed object, process and delete it. It may be that the std::vector would have to be adapted to make this syntax cleaner. \$\endgroup\$
    – Edward
    Jun 16, 2018 at 22:52
  • \$\begingroup\$ As for your final comment, yes, for this extract inheritance isn't really needed. I'd be happy to post a larger extract, but it's hard to figure out how to pull out something coherent that still faithfully represents the total actual program without posting thousands of lines of code. I may well do another attempt at an extract, though, as mentioned in my question. Thanks for the comments! \$\endgroup\$
    – Edward
    Jun 16, 2018 at 22:55
  • \$\begingroup\$ @Edward You're welcome! I butchered the first code snippet because I had changed the signature of eat, duh - I'll correct that in just a second. \$\endgroup\$ Jun 17, 2018 at 10:40
  • \$\begingroup\$ About your comment on the signature of eat, shouldn't void eat(unique_ptr<Food>) better fit in this context? \$\endgroup\$ Jun 18, 2018 at 13:02
  • \$\begingroup\$ @MathiasEttinger No, because that effectively bars eat from being used with anything that is not managed by a unique_ptr, e.g. an object on the stack. I'm not a big fan of passing smart pointers around like that, unless you can really justify that you never need to accept anything that is not a smart pointer. \$\endgroup\$ Jun 18, 2018 at 13:27

Your Answer

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

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