[prog.flame.c++] А потом говорят, что C++ ущербный язык...

По следам недавней темы заглянул в кишки проекта disruptor-cpp. На первый взгляд, это калька Disruptor-а с Java на C++. Вроде как и сделанная нормально. Но, в то же время, хороший пример того, что не нужно делать в современном C++. И об этом чуть подробнее под катом.

Так уж получилось, что первым файлом, куда я заглянул, оказался файл ring_buffer.h. Для простоты просто приведу код шаблонного класса RingBuffer целиком, благо его немного:

// Ring based store of reusable entries containing the data representing an // event beign exchanged between publisher and {@link EventProcessor}s. // // @param <T> implementation storing the data for sharing during exchange // or parallel coordination of an event. template<typename T> class RingBuffer : public Sequencer {  public:     // Construct a RingBuffer with the full option set.     //     // @param event_factory to instance new entries for filling the RingBuffer.     // @param buffer_size of the RingBuffer, must be a power of 2.     // @param claim_strategy_option threading strategy for publishers claiming     // entries in the ring.     // @param wait_strategy_option waiting strategy employed by     // processors_to_track waiting in entries becoming available.     RingBuffer(EventFactoryInterface<T>* event_factory,                int buffer_size,                ClaimStrategyOption claim_strategy_option,                WaitStrategyOption wait_strategy_option) :             Sequencer(buffer_size,                       claim_strategy_option,                       wait_strategy_option),             buffer_size_(buffer_size),             mask_(buffer_size - 1),             events_(event_factory->NewInstance(buffer_size)) {     }     ~RingBuffer() {         delete[] events_;     }     // Get the event for a given sequence in the RingBuffer.     //     // @param sequence for the event     // @return event pointer at the specified sequence position.     T* Get(const int64_t& sequence) {         return &events_[sequence & mask_];     }  private:     // Members     int buffer_size_;     int mask_;     T* events_;     DISALLOW_COPY_AND_ASSIGN(RingBuffer); };

Это, на минуточку, код для C++11. Но полное ощущение, что человек осваивал C++ еще в те времена, когда STL-я вообще не было.

Главная моя претензция -- это голый указатель events_. В наше время оправданий для работы с голыми указателями в C++ очень мало. В данном случае их вообще нет.

Во-первых, от голого указателя можно было бы избавиться посредством std::vector. Метод EventFactoryInterface<T>::NewInstance() возвращал бы std::vector<T> по значению. За счет move semantics возвращенное из NetInstance() значение перемещалось бы в event_ без лишних накладных расходов. И все! Никаких new внутри фабрики. Никаких деструкторов в самом RingBuffer-е.

Кстати о том, что вместо new T[N] в современном C++ нужно использовать std::vector<T>(N) Бьёрн Страуструп писал еще в третьем издании "Язык программирования C++" в самом-самом начале 2000-х, т.е. почти 15 лет тому назад.

Во-вторых, если std::vector по каким-то религиозным причинам не проходит, то почему нельзя было задействовать std::unique_ptr? В С++11 std::unique_ptr, в отличии от std::auto_ptr из C++98/03, совершенно спокойно поддерживает T[].

Далее. Почему аргумент event_factory передается в конструктор по указателю? Чтобы nullptr подсунуть проще было? Или у автора стойкая нелюбовь к ссылкам?

Аналогичный вопрос можно задать и по поводу возвращаемого значения метода Get. Какой смысл возвращать указатель и почему нельзя было вернуть ссылку?

Однако, самый главный пакет претензий вызывает то, что связано с заданием размера буфера.

Начнем с того, что в C++, в отличии от Java, есть беззнаковые целые типы. И размерности векторов, а так же индексы элементов в них принято задавать посредством безннаковых целых типов. Причем желательно не просто unsigned int, а std::size_t, дабы не иметь проблем при переходе между 16-/32-/64-битовыми платформами.

Далее, многие ли обратили внимание, насколько критично требование о том, чтобы значение buffer_size было степенью двойки? Кстати, если я правильно помню, единица -- это тоже степень двойки, нулевая :)

Но нигде в сигнатуре конструктора RingBuffer нет даже попытки придать этому требованию более жесткий и формальный вид. Хотя в C++, в отличии от Java, сделать это вообще не проблема. Основы этой техники идут в Виртовский Pascal, где можно было объявлять подтипы существющих типов. Этой же технике было посвящено несколько слайдов в презентации про использование Haskell-я в Standard Chartered Bank (слайды с 16-го по 20, как минимум).

Всего-то и нужно было -- определить махонький вспомогательный класс:

struct SizeAsPowerOfTwo {    const std::size_t v;    SizeAsPowerOfTwo( std::size_t a )       :  v( EnsureValidity( a ) )    {}    inline static std::size_t    EnsureValidity( std::size_t a )    {       if( a <= 1 || (0 != (a & (a-1))) )          throw std::invalid_argument( std::to_string(a) +                ": is not a valid buffer size" );       return a;    } };

И использовать тип size_as_power_of_two в качестве типа параметра buffer_size в конструкторе RingBuffer-а.

Ну и еще одно отличие C++ от Java -- атрибуты класса можно делать константными. В данном случае атрибуты buffer_size_ и mask_ следовало бы сделать таковыми, т.к. класс RingBuffer не предполагает их изменения в принципе.

С константностью связан еще один момент вокруг метода Get. По идее, его тоже можно бы объявить константным, т.к. вызов Get не должен менять содержимое RingBuffer-а. Однако, если Get будет константным, а event_ будет представлен как std::vector, то event_ нужно будет помечать как mutable-атрибут класса, иначе из Get-а нельзя будет вернуть неконстантную ссылку на T. Кого-то может смущать наличие mutable-атрибута в классе. Кроме того, может быть RingBuffer-у не помешало бы вообще иметь два метода Get -- константный и неконстантный. Здесь нужно отталкиваться от сценариев использования.

Ну и еще, если говорить про метод Get, не факт, что sequence туда нужно передавать по ссылке. Метод inline-овый, sequence используется один раз внутри арифметического выражения. Вполне возможно, что передача по значению была бы даже эффективнее. Так что я бы сначала сделал передачу по значению, а уже если профайлер укажет, что здесь есть просадка, вернулся бы к передаче по ссылке.

Кроме того, если уж мы перешли от int-а к std::size_t, то и тип аргумента sequence для Get нужно преобразовать к uint64_t, дабы не было смешения знаковых и беззнаковых значений в одном выражении.

Итого, если собрать все вместе, то следовало бы написать что-то вроде (предупреждаю, это не компилировалось):

Upd. Атрибут buffer_size_ вообще был убран за ненадобностью.

// Ring based store of reusable entries containing the data representing an // event beign exchanged between publisher and {@link EventProcessor}s. // // @param <T> implementation storing the data for sharing during exchange // or parallel coordination of an event. template<typename T> class RingBuffer : public Sequencer {  public:     // Construct a RingBuffer with the full option set.     //     // @param event_factory to instance new entries for filling the RingBuffer.     // @param buffer_size of the RingBuffer, must be a power of 2.     // @param claim_strategy_option threading strategy for publishers claiming     // entries in the ring.     // @param wait_strategy_option waiting strategy employed by     // processors_to_track waiting in entries becoming available.     RingBuffer(EventFactoryInterface<T> &  event_factory,                SizeAsPowerOfTwo buffer_size,                ClaimStrategyOption claim_strategy_option,                WaitStrategyOption wait_strategy_option) :             Sequencer(buffer_size,                       claim_strategy_option,                       wait_strategy_option),             mask_(buffer_size.v - 1),             events_(event_factory.NewInstance(buffer_size.v)) {     }     // Get the event for a given sequence in the RingBuffer.     //     // @param sequence for the event     // @return event pointer at the specified sequence position.     const T& Get(uint64_t sequence) const {         return events_[sequence & mask_];     }     // Get the event for a given sequence in the RingBuffer.     //     // @param sequence for the event     // @return event pointer at the specified sequence position.     T& Get(uint64_t sequence) {         return events_[sequence & mask_];     }  private:     // Members     const std::size_t mask_;     std::vector<T> events_;     DISALLOW_COPY_AND_ASSIGN(RingBuffer); };

Ну и здесь еще имело бы смысл посмотреть на то, а следует ли RingBuffer защищать от копирования и присваивания. Хотя бы потому, что от этого уже защищен его базовый класс, Sequencer. Так что я бы еще убрал бы и использование макроса DISALLOW_COPY_AND_ASSIGN.

---

Зачем все это было написано? Да просто накатывает временами. В C++ уже лет двадцать как есть нормальные средства, чтобы писать с минимальными шансами отстрелить себе что-нибудь. И чем старше C++, тем больше у него этих средств и возможностей. Но как посмотришь на какой-нибудь недавно написанный код, так блин, реальный привет из конца 80-х. Ну нельзя же так, в конце-то концов.

PS. Трюк с определением того, что число является степенью двойки был найден по ссылкам из старого поста.