@rotor28 ноя 2014 в 21:35

Фабричный метод без размещения в динамической памяти

8 мин

18K

Ненормальное программирование * C++ * Проектирование и рефакторинг * ООП *

+26

Комментарии 49

@encyclopedist 28 ноя 2014 в 22:00

Фактически, вы изобрели вариант, вроде boost::variant. А использованная конструкция называется tagged union. Для упрощения создания таких штук в C++11 предусмотрели std::aligned_union (кстати, вопрос выравнивания у вас не затронут).

@encyclopedist 28 ноя 2014 в 22:12

Ну и вообще, в C++11 есть relaxed unions: разрешено иметь объединения, содержащие любые типы, но необходимо в ручную вызывать placement new и деструктор (собственно, вы такое объединение объявили, но использовали почему-то только для вычисления размера, но не напрямую для хранения элементов). Собственно, массив mem не нужен, а достаточно поместить объединение в класс. Почитать

@rotor 29 ноя 2014 в 06:49

В этом случае как раз и получится то, что у меня представлено во втором варианте. Дело в том, что 1) доступ к членам union осуществляется по их именам, а не типам, и 2) мы не сможем создать объект такого объединения, если он будет содержать более одного не POD типа с нетривиальным конструктором. Попробуйте сами.

@encyclopedist 29 ноя 2014 в 10:21

Как минимум в GCC 4.8 и 4.9 работает. А вы какой компилятор используете?

@rotor 29 ноя 2014 в 16:54

У меня тоже компилируется. Я использую clang 3.2-7 и gcc 4.8.1. Ваш пример с Variant я посмотрю. Пока просто не успел. Обязательно разберусь с этим подходом.

@encyclopedist 29 ноя 2014 в 11:01

А на счёт доступа по именам — это решается с помощью рекурсивного объединения. По ссылке, которую я привел, описан весь процесс создания Variant таким методом.

@Mingun 30 ноя 2014 в 16:02

Сделал перевод статьи и оставляю этот комментарий в качестве перекрёстной ссылки для заинтересовавшихся.

@rotor 30 ноя 2014 в 17:58

Круто! Вы успели перевести быстрее чем я прочитать.

@rotor 29 ноя 2014 в 06:41

Спасибо за интересные ссылки.
По поводу выравнивания. Так ведь sizeof возвращает размер с учетом выравнивания. Так что не вижу с этим проблемы. Или какой то еще подвох здесь есть?
На счет std::aligned_union согласен, вместо unsigned char mem[usize] лучше использовать его.

@phprus 29 ноя 2014 в 07:40

sizeof возвращает размер с учетом выравнивания после объекта.
А для корректной работы может понадобиться выравнивать начало размещения объекта в памяти по границе, которая не совпадает со стандартным выравниванием начала массивов char.

@rotor 29 ноя 2014 в 09:16

Ага понял. Тогда да, — это грабли. Ладно, вечерком посмотрю, подправлю.

@antoshkka 30 ноя 2014 в 11:33

Фактически, вы изобрели вариант, вроде boost::variant.

С boost::variant код выглядел бы следующим образом и имел бы следующие плюсы:

class BaseCreator
{
    // boost::none_t - чтобы иметь пустое состояние и конструктор по умолчанию без накладных расходов
    typedef boost::variant<boost::none_t, A, B> obj_t;
    obj_t obj;

public:
    BaseCreator() = default; // noexcept

    BaseCreator(bool x)
        : obj(x ? obj_t(A()) : obj_t(B()))
    {}

    // Можно не запрещать копирование :)
    BaseCreator(const BaseCreator &) = default;
    // Перемещение
    BaseCreator(BaseCreator &&) = default;

    Base* operator->()
    {
        // polymorphic_get был добавлен в Boost 1.56. Рекомендую к употребелению! Классная штука.
        //
        // Если это единственное место где используется Base, то 
        // деструктор можно не делать виртуальным.
        //
        // Кстати, Ваш пример будет некорректно работать в случае сложного множественного наследования:
        // так reinterpret_cast<T*>(mem); не справится с struct A: virtual Base1, virtual Base {...}
        return &boost::polymorphic_get<Base>(obj);
    }
};

@lemelisk 30 ноя 2014 в 13:14

Код примера и с обычным множественным наследованием работать не будет, достаточно, чтобы адрес базового класса не совпадал с адресом начала объекта.

@rotor 30 ноя 2014 в 18:13

Да, получилось весьма симпатично.

@kosmos89 29 ноя 2014 в 01:37

Какая-то странная фабрика. Может вернуть только один объект за раз. И конструктор вызовется непонятно когда. Вызывающий не может детерминировано удалить его. Почему вообще не сделать пул, если уж на то пошло?
Не представляю, где его можно на практике применить. Может приведете пример?

@kosmos89 29 ноя 2014 в 01:53

>И конструктор вызовется непонятно когда.
Имел в виду деструктор.

@rotor 29 ноя 2014 в 07:09

Может вернуть только один объект за раз

Потому что это не «фабрика», а «фабричный метод». Собственно переделать всю конструкцию в фабрику по общему принципу тоже можно. Просто на примере фабричного метода проще проиллюстрировать подход.
Деструктор явно вызывать не нужно, т.к.стековые объекты существуют лишь в границах содержащего их блока. Собственно с классическим фабричным методом тоже самое, если он возвращает не сырой указатель, а, как положено, unique_ptr.
Что касается применения, то оно ни чем не отличается от применения обычного фабричного метода с динамическим размещением. См. первый попавшийся пример из гугла

@Wyrd 29 ноя 2014 в 11:55

Деструктор вызвать все-таки нужно. Не нужно освобождать память.

@rotor 29 ноя 2014 в 16:55

Так он и вызывается автоматически. Во всех примерах. Гляньте еще разок.

@kosmos89 29 ноя 2014 в 13:52

Деструктор нельзя не вызывать. Может там ресурсы какие-то освобождаются.

>Потому что это не «фабрика», а «фабричный метод».
Ну и что? Creator может несколько объектов запросить.

@Lol4t0 29 ноя 2014 в 14:05

Да потому что это не фабричный метод, а этакой pimpl

@Zigmar 29 ноя 2014 в 01:51

Очень удивился, увидев non-POD типы в union, но потом сообразил, что видимо речь идет про С++11.

@ilammy 29 ноя 2014 в 10:24

То есть std::unique_ptr, override и -std=c++11 вас не смутили в первом же сниппете? :)

@Lol4t0 29 ноя 2014 в 11:55

У вас большие проблемы с выравниванием и копированием/переносом.

Нельзя просто взять и скопировать объект в C++, скопировав его данные. Нужно еще вызвать его конструктор копирования.

И вообще нужно просто использовать small obejct allocator-ы, и не париться.

@rotor 29 ноя 2014 в 17:14

Копирование запрещено. На счет конструктора перемещения, похоже, вы правы. С TypeUnion(TypeUnion &&) = default я поторопился. Еще одни грабли.

@rotor 29 ноя 2014 в 21:26

Сейчас еще раз подумал. Мы же добиваемся такого же поведения, как и при использовании динамического выделения. Так что, запрещенный конструктор копирования и дефолтный конструктор переноса — то что надо. Собственно unique_ptr так и работает.
Таким образом, с копированием/переносом проблем нет.

@Lol4t0 29 ноя 2014 в 21:55

Не добиваемся мы. Для того, чтобы добиться этого, надо чтобы объект не перемешался при перемещении `BaseCreator`-а. `unique_ptr` работает именно _так_, а ваш код — не так.

@rotor 29 ноя 2014 в 22:58

Да, каюсь. Глупость ляпнул.

@lemelisk 29 ноя 2014 в 22:18

Дефолтный конструктор перемещения BaseCreator просто вызывает конструктор перемещения TypeUnion, и это адекватное поведение. Но вот конструктор перемещения TypeUnion, вместо того чтобы вызвать конструктор перемещения того объекта, который в нем в данный момент хранится, тоже объявлен дефолтным, а это значит, что он просто копирует (через memcpy) внутренний буфер и указатель. Если в TypeUnion будет хранится, например, unique_ptr, то это приведет к тому, что у вас станет два unique_ptr'а, владеющие одной и той же памятью, в результате чего, например, после вызова деструкторов обоих TypeUnion один и тот же участок памяти будет освобожден дважды.

@lemelisk 29 ноя 2014 в 16:02

Интересный факт, что ваш код (который под спойлером "Полный код примера") крашится во время работы, если скомпилировать его на GCC 4.9.0 c флагом -O2: coliru.stacked-crooked.com/a/1742a8366403abef Если компилировать clang++, GCC 4.8.2 или понизить флаг до -O1, то все работает как надо. Не готов сказать баг ли это компилятора, или у вас какое-то хитрое UB в коде, проблема в вызове p->f() и она как-то связана с инлайном, потому что с ключами -O2 -fno-inline все начинает работать.

@rotor 29 ноя 2014 в 17:32

Я думаю UB более вероятно :)
На моих clang 3.2-7 и gcc 4.8.1 я воспроизвести не могу. Но по вашей ссылке видно, что выход происходит в строке 72

   assert ( deleter ); // TypeUnion::assign was not called

Т.е. каким то образом deleter == nullptr. Попробую найти GCC 4.9.0 и отдебажить.
Еще чуть выше мне подсказали о возможных проблемах с выравниванием. Может как то с этим связано. Короче, надо будет разобраться.

@lemelisk 29 ноя 2014 в 18:36

assert там не виноват, если его закомментировать, все равно будет падать. Выравнивание я поправлял, не помогло.

@rotor 29 ноя 2014 в 18:42

Ну да assert просто сигнализирует, что не был вызван assign. Который инстацирует объект. Если объект не инстацирован, то падать оно конечно будет т.к. в памяти не то что мы ожидаем. Просто такого там быть не должно. Но в g++-4.9.2 проблема не воспроизводится.

@rotor 29 ноя 2014 в 18:38

Только что прогнал по g++-4.9.2 c разными флагами. Все ок. Никаких проблем.

@lemelisk 29 ноя 2014 в 18:47

Странно это, извините за беспокойство. Просто хотел кое-что проверить, закинул ваш пример в онлайн-компилятор, а там такое вот…

@rotor 29 ноя 2014 в 19:47

Да нет, все нормально. Мне самому интересно. Сижу вот до сих пор дебажу прямо по вашей ссылке. Надо же выяснить в чем там дело.

@Mingun 30 ноя 2014 в 11:51

Если все же найдете проблему, не забудьте поделиться с общественностью.

@Lol4t0 30 ноя 2014 в 14:57

Видимо, все дело тут в некорректном reinterpret_cast в функции get, который приводит к UB, как сообщают нам antoshkka и lemelisk чуть выше

@lemelisk 30 ноя 2014 в 15:14

Он некорректен только в случае множественного наследования, то есть когда у одного класса сразу несколько базовых. У вас в примере таких классов нет. С множественным наследованием проблема в том, что когда у объекта несколько базовых классов, то адрес одного из них будет не совпадать с адресом начала объекта, и для корректного преобразования нужно будет не только поменять тип указателя, а ещё и сдвинуть его на некоторую величину.

@Lol4t0 30 ноя 2014 в 15:27

Вы сохранили B*, а прочитали Base* — это UB, по определению. А в нормальных программах не бывает UB. А раз тут нет UB, значит этот код никогда не выполнится. А это возможно только тогда, когда assert всегда срабатывает. А раз assert всегда срабатывает, то и не фиг его проверять. Именно так и поступил gcc, там __assertion_failed вызывается безусловно. Можете посмотреть вывод ассембелра.

@lemelisk 30 ноя 2014 в 15:44

А почему это UB? Ведь Base — базовый класс B. Если заменить obj.get<Base>(); на obj.get<B>(); от краша все равно не спасает.

@Lol4t0 30 ноя 2014 в 15:50

Хм. Возможно, бага компилятора в этом месте. Потому что замена , return reinterpet_cast<...> на return new B() таки от креша спасет, так что я уверен, что логика его именно в том, что reinterpet_cast приводит к UB.

@rotor 30 ноя 2014 в 18:19

Обязательно. Но вчера я так и не нашел причину. Сложность еще и в том, что «дебажить» приходится в браузере, что весьма необычно для меня. (у меня нет g++-4.9.0, а только g++-4.9.2 и g++-4.8.1)

@rotor 30 ноя 2014 в 20:01

Нашел баг!!! coliru.stacked-crooked.com/a/d919304815452d35
Все таки не зря encyclopedist с самого начала сказал о проблемах с выравниванием, а я вместо того что бы устранить грабли принялся дебажить изначально дефективный код.
Если кратко, как исправлен баг:
Добавлено

template <std::size_t Len, typename ...Types>
struct aligned_union {
    static constexpr size_t max(size_t r0) {return r0;}
    template <typename ...R> static constexpr size_t max(size_t r0, R... r) {return ( r0 > max(r...) ? r0 : max(r...) );}
    static constexpr std::size_t alignment_value = max(alignof(Types)...);
    struct type { alignas(alignment_value) unsigned char _[max(Len, sizeof(Types)...)]; };
};

т.к. в этой версии gcc, видимо, std::aligned_union уже удален.

    unsigned char mem[usize];

заменено на

    typedef typename aligned_union<usize, Types...>::type aligned_type;
    aligned_type _storage;
    aligned_type *mem = &_storage;

Таким образом, проблема решилась добавлением выравнивания.

В моем случае можно было поступить и проще. Просто заменить unsigned char mem[usize]; на

alignas(max(alignof(Types)...)) unsigned char mem[usize];

@lemelisk 30 ноя 2014 в 20:30

Нет, дело не в выравнивании (я его исправлял, не помогает), вы добавили лишний член — указатель mem, просто чтобы хранить в нем адрес другого члена этого же объекта (_storage), и магическим образом это помогло. Замените в функции get return static_cast<T*>(static_cast<void*>(mem)); на return static_cast<T*>(static_cast<void*>(&_storage)); и снова начнет крашится.

@Lol4t0 30 ноя 2014 в 20:31

Не, это не считается. Вы слишком сильно переписали код.

Если заменить ваш unsigned char mem[usize]; на alignas(max(alignof(Types)...)) unsigned char mem[usize];, то делу это не поможет, все равно будет падать. Так что проблема не в alignment-е. Более того, если написать

unsigned char mem_s[usize];
void* mem = mem_s;

то этого будет достаточно, чтобы компилятор не включал ~~дурку~~оптимизацию. Но дело в том, что если компилятор шалит, то либо у вас UB, либо в компиляторе ошибка. Когда вы так сильно переписали код, то в первом случае вы могли только спрятать UB, и поэтому это не может считаться решением пока вы не ответили, что конкретно в вашем коде было не так.

@encyclopedist 30 ноя 2014 в 20:36

aligned_union не удален, он наоборот, ещё не добавлен. Он будет добавлен только в GCC версии 5. (Я при написании первого комментария, честно говоря, подзабыл о том, что оно ещё недоступно в GCC, хотя сам несколько мес. назад сталкивался с этим)

@yomayo 1 дек 2014 в 11:46

Хранение объектов в «куче» — накладные расходы. Если хочется объекты переменной длины размещать в стеке, то можно ещё использовать такую технику: «Размещение объектов переменной длины с использованием двух стеков», «Реализация двухстековой модели размещения данных». Правда, она требует ассемблерных вставок.

@ilammy 1 дек 2014 в 19:36

Насколько я понимаю, никакого «особого, аппаратного стека» не существует. Просто call, ret, enter, leave, push, pop неявно используют ebp и esp. Поэтому и ассемблерных вставок в общем-то не надо, чтобы иметь глобальный динамический стек под объекты.

Как самый простой вариант: alloca() — неявно выделяет память прямо на «родном» esp-стеке. Освобождает при выходе из функции. Деструкторы можно навесить, завернув такие объекты в умный указатель. Однако, alloca() considered harmful, потому что обычно у стека весьма скромный объём. Поэтому чуть более сложный вариант: засунуть в thread-local storage указатель на кусок глобальной памяти под собственный стек и написать свою alloca(), которая выделяет память оттуда.

Зарегистрируйтесь на Хабре, чтобы оставить комментарий