Core style guide

[dotnwat]

Forward declarations

Headers

Subsystems should define a fwd.h header containing forward declarations for exported components. For example raft/fwd.h contains the following forward declarations.

#pragma once

namespace raft {

class consensus;
class group_manager;
class recovery_throttle;

} // namespace raft

Then users of the exported components may include subsystem/fwd.h rather than the header containing the full class declaration. An important case to remember is that seastar::sharded<T>& will work with forward declarations:

#include "raft/fwd.h"

class object {
...
    seastar::sharded<group_manager>& _pm;
};

With std::unique_ptr

A class with a std::unique_ptr<T> member can use a forward declaration of T if the destructor is out of line:

e.g. in application.h:

#include <cluster/fwd.h>

class application {
public:
    ...
    application::~application();
private:
    ...
    std::unique_ptr<cluster::controller> controller;
};

And in application.cc:

application::~application() = default;

Rationale

1. Take pressure off the pre-processor and compiler which would otherwise be required to process a full header (and transitively included headers) when a forward declaration would suffice.
2. Help reduce unnecessary recompilation of dependencies

std::function vs seastar::noncopyable_function

Rationale

Emulating seastar::future::finally with coroutines

Rationale

seastar::sleep vs seastar::sleep_abortable

Rationale

[BenPope]

Forward declarations

A class with a std::unique_ptr<T> member can use a forward declaration of T if the destructor is out of line:

e.g. in application.h:

#include <cluster/fwd.h>

class application {
public:
    ...
    application::~application();
private:
    ...
    std::unique_ptr<cluster::controller> controller;
};

And in application.cc:

application::~application() = default;

[dotnwat]

Nice. I'll fold this into the above

[dotnwat]

Done

[BenPope]

Runtime polymorphism

When runtime polymorphism is required, prefer non-virtual public interface:

// Header
class polymorphic final {
public:
    struct impl {
        virtual ~impl() = default;
        virtual void apply() = 0;
    };

    explicit polymorphic(std::unique_ptr<impl> impl)
      : _impl(std::move(impl)) {}

    void apply() { _impl->apply(); }

private:
    std::unique_ptr<impl> _impl;
};

polymorphic make_derived();

// Implementation
class derived_impl final : public polymorphic::impl {
    void apply() final {}
};

polymorphic make_derived() {
    return polymorphic{std::make_unique<derived_impl>()};
}

Notes

1. The derived class is marked final - the optimiser may be able to do devirtualization.
2. The impl is held by unique_ptr which means derived is movable but noncopyable.
3. Copy and assignment can be implemented to make it a regular type.

Rationale

1. Inheritance based polymorphism implies heap allocation, placing the burden of lifetime management with the user.
2. That often leads to using the type via a (smart) pointer; the user has to do the dereference at point of use.
3. Pointers often lead to unintended sharing, an incidental data structure, and spooky action at a distance.
4. Sometimes reference types are used like value types, so there's either slicing, or a virtual clone method, which is unsatisfactory.
5. Regular types are a lot easier to use and reason about.

[BenPope]

Rationale

1. Inheritance based polymorphism implies heap allocation, placing the burden of lifetime management with the user.
2. That often leads to using the type via a (smart) pointer; the user has to do the dereference at point of use.
3. Pointers often lead to unintended sharing, an incidental data structure, and spooky action at a distance.
4. Sometimes reference types are used like value types, so there's either slicing, or a virtual clone method, which is unsatisfactory.
5. Regular types are a lot easier to reason about.

[dotnwat]

Thanks that's really clear. I still need to watch this video, too!

[BenPope]

There have been quite a few talks and libraries based on the concept. They generally refer to type-erasure.

This should be quicker than the talk: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Type_Erasure

[travisdowns]

@BenPope - I've used https://github.com/jbcoe/polymorphic_value for this and found it very nice. Gives you a regular class with runtime polymorphism without needing the boilerplate above.

[felixguendling]

1. Inheritance based polymorphism implies heap allocation, placing the burden of lifetime management with the user.

As I understand it correctly, the example code also makes a heap allocation: you need to give the class a unique_ptr<T>.

the optimiser may be able to do devirtualization

This also applies to normal virtual inheritance. Especially if you enable LTO, there are very good chances, it will be optimized.

polymorphic make_derived() {
    return polymorphic{std::make_unique<derived_impl>()};
}

This has to be implemented for every ctor any derived type derived_impl offers? That can be a lot of extra boilerplate code.

I think it's hard to make a general rule what to use. Especially in performance-sensitive code there's no "one size fits all" in my experience. There are many cases where data oriented design (https://www.youtube.com/watch?v=rX0ItVEVjHc) yields incredible performence. Sometimes real runtime polymorphism isn't even needed and you can just use a template <typename T> with duck-typing at compile time. Also std::variant<T> offers higher performance than virtual inheritance based polymorphism in many cases. Sometimes you can have significant speed gains by using a custom allocator (saving time allocating but also deallocating!) - the proposed design makes it hard to have a custom allocator. If performance is not important, I think in most cases it's totally fine to simply use what C++ already offers out of the box and make things not more complicated. I never had problems with using smart pointers for memory management directly and passing around references const& or & of those types.

One advantage of using https://github.com/jbcoe/polymorphic_value over the proposed solution is that it already implements all kinds of operators including const T* operator->() const (and non-const) to access the underlying class functions. It also offers customization points for copy and delete.

[BenPope]

Name Lookup

Hidden friends

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1601r0.pdf

With hidden friends the function lookup is found by ADL only; i.e., it cannot be found by unqualified lookup.

E.g. implement operators as friends, ideally inline:

namespace bar {
struct foo {
    int i;
    friend std::ostream& operator<<(std::ostream& os, const foo& f) {
        return os << f.i;
    }
};
// namespace bar

Notes

foo will be found by ADL, there's no need to also declare or define it at namespace scope. It can also be a private member; that doesn't affect name lookup.

Rationale

• Prevents implicit conversions
• Reduces overload set
  • Reduces error message length when an overload is not found
  • Reduces compile time

[BenPope]

Customization Point Design

When you want a behaviour to be extensible.

Customisation Point Object with tag_invoke

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2221r0.pdf

Rationale

[BenPope]

@felixguendling I'd love your opinion here. I've been meaning to find time to play with serde, and specifically with how the customisation points should work.

[felixguendling]

In serde I made use of pure ADL. I think we need to distinguish between library code (relatively little code that's used everywhere) and application code (most of the code base for a codebase like redpanda). However, usually when talking about library code, people assume that library code is code that cannot be changed by the people who develop the application code - which is not true in our case. So I don't think that general rules of writing library code need to apply here.

I would also say that a good rule of thumb (not just for this CPD issue) is to "keep it simple". Simple ADL requires less knowledge (you only need to know that the compiler also considers functions from the namespaces of the arguments) and since we have control over the code base (both library and application code), the collision problem (one problem solved by tag_invoke) is easy to solve. So the benefits of tag_invoke are not big enough to force everything to be implemented with the tag_invoke pattern.

Btw the paper you linked is a discussion of one concrete application of the pattern for the executors. I think this paper explains the basics better: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1895r0.pdf

[dotnwat]

@felixguendling these few paragraphs here are really clear and enlightening. thanks for writing them.

[BenPope]

#9132

[dotnwat]

Access Specifiers

Ordering

class type {
public:
protected:
private:
};

Duplicate sections

It's okay to use multiple instances of access specifiers to create logical groupings where needed.

class type {
public:
   // boiler plate

public:
  // logical grouping of APIs

...
};

cc @michael-redpanda @graphcareful @rockwotj

[dotnwat]

Closing in favor of driving consensus through PR discussions #14812