1

Let's assume, that we have AbstractBufferBuilder class that contains some common methods for building some Buffer classes:

class AbstractBufferBuilder {
public:

    AbstractBufferBuilder &setBufferData(...) {
        //some code
        return *this;
    }
};

And we have ConcreteBufferBuilder class:


class ConcreteBufferBuilder : public AbstractBuilder {
public:

    ConcreteBufferBuilder &addAttribute(...) {
        //some code
        return *this;
    }

    Buffer* build(){
        return new Buffer(...);
    }
};

To create new Buffer class, we can do

ConcreteBufferBuilder().setBufferData(...).addAttribute(...).build();

But this code won't be compiled because setBufferData() returns AbstractBufferBuilder that doesn't contain addAttribute().

I've tried to use templates to solve this problem:

template<class Builder>
class AbstractBufferBuilder {
public:

    Builder &setBufferData(...) {
        //some code
        return *dynamic_cast<Builder*>(this);
    }
};
class ConcreteBufferBuilder : public AbstractBuilder<ConcreteBufferBuilder> {
public:

    ConcreteBufferBuilder &addAttribute(...) {
        //some code
        return *this;
    }

    Buffer* build(){
        return new Buffer(...);
    }
};

It works, but templates give extra work to the compiler and linker, so it increases build time and file size.

Another possible solution is to use a wrapper for AbstractBufferBuilder methods:


class AbstractBufferBuilder {
protected:

    void setBufferDataImpl(...) {
        //some code
    }
};
class ConcreteBufferBuilder : public AbstractBuilder {
public:
    ConcreteBufferBuilder &setBufferData(...){
        AbstractBuilder::setBufferDataImpl(...);
        return *this;
    }

    ConcreteBufferBuilder &addAttribute(...) {
        //some code
        return *this;
    }

    Buffer* build(){
        return new Buffer(...);
    }
};

But then we need to write same code every time we create a new Builder class which is also not good.

Of course, we can abandon the use of chain calls, but I want to solve this particular problem.

Same question already asked: Builder Pattern and Inheritance, but my question is little bit different.

Is there any better solution to this problem?

AlexusXX
  • 15
  • 4
  • With the template version, I'd give the `AbstractBufferBuilder` a `virtual Builder& self()=0` method, and then its methods can `return self();`, and then you don't need any sketchy casts. There's a slight performance penalty though. – Mooing Duck Mar 15 '23 at 15:34
  • IMHO your template solution is great. It's classic [CRTP](https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern). You can actually `static_cast` instead of `dynamic_cast`. *"but templates give extra work to the compiler and linker, so it increases build time and file size"*: Are your requirements on build time and file size really **this** tight? Have you actually checked how big the penalty is? – joergbrech Mar 15 '23 at 16:52
  • 1
    @MooingDuck If you `static_cast` instead of `dynamic_cast`ing I don't think the cast is sketchy. On the contrary, adding a vtable lookup is an unnecessary runtime penalty. – joergbrech Mar 15 '23 at 16:56
  • By the way, you already provide two solutions and ask which one is better. Maybe the question is better suited for https://codereview.stackexchange.com? – joergbrech Mar 15 '23 at 16:57
  • @joergbrech I'm writing a rendering engine for my game using OpenGL and for my convenience I decided to wrap some OpenGL calls into classes. Builder helps me to build buffers easily and I guess that such Builders will be used in many other cases (shaders, framebuffers, etc). Therefore, I think that using templates will significantly increase the build time and file size in the future. – AlexusXX Mar 15 '23 at 17:20
  • @joergbrech No, I'm asking if there are better ways to solve the problem than the ones I suggested – AlexusXX Mar 15 '23 at 17:23
  • I have a hunch you are optimizing too early. Firstly, for a game engine I would suspect runtime performance to always have priority over build time and file size. Secondly, I would be surprised if the build time and file size penalty of the CRTP approach is noticible in practice at all. You are depriving yourself of useful programming techniques (templates) without knowing for sure if they are problematic or not. I personally would implement it using CRTP and then (maybe, if I feel there is the need) benchmark a real world scenario later. If the costs were too high, refactor later. – joergbrech Mar 16 '23 at 05:55

1 Answers1

0

I do not have a perfect solution for this problem, but I can suggest some options:

  1. The first one is to stick to the CRTP as you did in your post. In order to minimize compile times and code duplication, the CRTP can be reduced to only the part that performs the casts:
class AbstractBufferBuilder {
public:
    virtual ~AbstractBufferBuilder() = default;

    AbstractBufferBuilder& setBufferData() {
        // Code does not have to be inline -- can live in a cpp file to diminish compile/link time
        return *this;
    }
};

template <typename DerivedSelf>
class AbstractBufferBuilderWrapper : public AbstractBufferBuilder {
public:
    DerivedSelf& setBufferData() {
        return static_cast<DerivedSelf&>(AbstractBufferBuilder::setBufferData());
    }
};

class ConcreteBufferBuilder : public AbstractBufferBuilderWrapper<ConcreteBufferBuilder>
{
public:
    ConcreteBufferBuilder& addAttribute() {
        return *this;
    }

    std::unique_ptr<Buffer> build() {
        return std::make_unique<Buffer>();
    }
};

int main() {
    auto buffer = ConcreteBufferBuilder {}
        .setBufferData()
        .addAttribute()
        .build();
}
  1. A second option is to drop the CRTP, but to use an explicit downCast() method to cast the result of setBufferData() back to a ConcreteBufferBuilder. The resulting code would look like this:
class AbstractBufferBuilder {
public:
    virtual ~AbstractBufferBuilder() = default;

    AbstractBufferBuilder& setBufferData() {
        return *this;
    }

    template <typename T, std::enable_if_t<std::is_base_of_v<AbstractBufferBuilder, T>, bool> = true>
    T& downCast() {
        assert(dynamic_cast<T*>(this));
        return *static_cast<T*>(this);
    }
};

class ConcreteBufferBuilder : public AbstractBufferBuilder
{
public:
    ConcreteBufferBuilder& addAttribute() {
        return *this;
    }

    std::unique_ptr<Buffer> build() {
        return std::make_unique<Buffer>();
    }
};

int main() {
    auto buffer = ConcreteBufferBuilder {}
        .setBufferData()
        .downCast<ConcreteBufferBuilder>()
        .addAttribute()
        .build();
}

Both have no runtime overhead (in Release mode anyway), but option 2. changes a bit the way the client should use the buffer creation API, and may potentially crash at run-time if an invalid down cast is performed (although both the std::enable_if and the assertion should catch common issues).

Edit: One can also reconsider the use of the builder pattern here and instead encode what you desire as data types. As an example:

struct BufferData {};
struct Attribute {};

struct BufferOptions {
    virtual ~BufferOptions() = default;
    virtual std::span<const Attribute> getAttributes() const = 0; // May be empty
    virtual std::optional<BufferData> getBufferData() const = 0;  // May be std::nullopt
};

struct ConcreteBufferOptions : BufferOptions {
    std::span<const Attribute> getAttributes() const { return attributes; }
    std::optional<BufferData> getBufferData() const { return data; }

    BufferData data;
    std::vector<Attribute> attributes;
};

std::unique_ptr<Buffer> createBuffer(const BufferOptions& options) {
    // TODO: Use the options to create the buffer
    return std::make_unique<Buffer>();
}
madmann91
  • 91
  • 4