Комментарии 51
Мда... странное у вас представление о простоте.
Это довольно распространённый способ оптимального хранения состояний. Настолько, что некоторые языки, как Swift например, имеют поддержку в стандартной библиотеке. Уверен, что и на C++ есть популярные и протестированные в бою библиотеки, чтобы не городить велосипеды самостоятельно.
Оно на самом деле просто. Это битые поля. Если нужны более детальные описания что-то, то можете спросить, я попытаюсь ответить.
Как по мне - так исходный код проще и наглядней. Сопровождать его в разы лучше, чем это нагромождение шаблонной магии.
Хотя как упражнение для ума... может быть интересно.
Булевы переменные в исходном варианте несомненно нагляднее, а вот с простотой уже не всё так однозначно. Это в примере все переменные в одном месте. А представьте себе разбросанное по классу, или даже классам, эти переменные. Тот же метод isActive может быть совсем в другом месте. И уже сопровождение не такое удобное.
Можно обойтись без шаблонной магии. И вообще без класса, используя только приватные методы. Шаблонная магия здесь только для проверок на этапе компиляции. Чтобы случайно не косячить
Повторюсь еще раз - как упражнение для ума - просто прекрасное решение. И, на мой взгляд, выполнено очень красиво.
Но использовать его в продакшен коде - очень сомнительное решение.
Причин несколько:
вместо кода, который определял набор флагов у нас теперь объект со всеми вытекающими последствиями.
при анализе ошибок мне теперь надо проанализировать не только набор условий, но и код создания объекта (а это шаблон, и не простой), и методы которые выдают решение условия.
ну и вишенка на торте - неявное ограничение на enum. В статье описано, что элементы enum должны быть нумерованы степенью 2-ки, но в коде это никак не отразить и на этом можно словить феерический баг.
Вот основные причины, по которым я считаю это решение переусложнённым.
P.S. И заметьте, я не говорю про увеличение времени компиляции, расход памяти и прочие радости.
Даже для велосипеда перебор получился. Почему не реализовать легкую обертку над std::bitset или использовать его напрямую? Или я чего-то не понял в условиях задачи?
В какую же хрень превратился современный С++ :( На такую простую конструкцию столько писанины, не делающей ничего полезного и нужной лишь для формального выполнения многочисленных требований и ограничений "правильного" с точки зрения последних стандартов кода.
Кстати вот здесь
constexpr void remove(EnumType condition) noexcept
{
value ^= static_cast<EnumUnderlyingType>(condition);
}не remove, а инверсия. Удаление это value &= ~static_cast❮EnumUnderlyingType❯(condition);
Ну а вообще есть битовые поля.
struct Conditions
{
unsigned Initialized : 1;
unsigned HasFocus : 1,
unsigned HasWindow : 1,
unsigned Visible : 1,
};И далее все просто
Conditions c;
c.Initialized = 1;
if(c.HasFocus) foo();
if(c.Visible && !c.HasWindow) bar();При желании можно еще завернуть в union и получить доступ ко всем полям сразу.
Дополню сразу
typedef union{
struct{
uint8_t initialized : 1;
uint8_t has_focus : 1;
uint8_t has_window : 1;
uint8_t visible : 1;
uint8_t reserved : 4;
}
uint8_t as_uint8;
} CONDITIONS;И далее можно выполнить одну проверку всех условий разом
//0x0F если для всех полей считаем 1 корректным значением
constexpr uint8_t CORRECT_ALL_CONDITIONS_VALUE = 0x0F;
inline bool IsAllConditionsCorrect(CONDITIONS cond){
return cond.as_uint8 == CORRECT_ALL_CONDITIONS_VALUE;
}Самое главное - проверять размер union'ов
А вы уверены, что в C++ эта штука не является UB?
ЕМНИП, обращение к неактивному элементу union-а в C++ -- это UB.
И еще я не уверен, что в C++ есть гарантия того, что распределение битовых полей идет от самых младших битов к старшим. Вроде как ничего не мешает распределять и от старших битов к младшим и это зависит от платформы и компилятора.
Да, вы во всём правы. Для С++ это всё действительно характерно. Надо свыкаться с тем, что С++ и С уже очень сильно разошлись
Тут все сводится к вопросу шашечки нам нужны или ехать:) На практике компиляторов по пальцам руки пересчитать (гцц, мсвц, шланг, несколько штук для микроконтроллеров..., и обычно они все работают одинаково). А в большинстве случаев проект вообще заточен под какой-то единственный конкретный компилятор.
Тут все сводится к вопросу шашечки нам нужны или ехать
Похоже, вы сейчас пытаетесь мне объяснить, что UB в C++ном коде -- это нормально? Ну ну, ну ну.
Компилятор - штука детерминориванная. Или вы хотите сказать что встречая UB компилятор каждый раз обращается к генератору случайных чисел чтобы решить какой код генерировать? От младших битов к старшим или наоборот размещать битовые поля?
Помимо разных компиляторов существуют разные версии, разные опции и разные условия.
Подозреваю, что если кто нибудь в MSVC или GCC внезапно поменяет порядок битов в битовых полях, это будет проблема покруче чем проблема 2038 года".
Впрочем, то что комитет до сих пор этот порядок не стандартизировал, а вместо этого заставляет людей писать тонны бессмысленного бойлерплейтного кода, не делает им чести. Возможно существуют какие-то экзотические компиляторы, где порядок битов другой, и поэтому не хотят стандартизировать? Для этого в том же стандарте прописать некое требование к разработчикам компиляторов, чтобы они выпускали документы, в которых четко прописывали, как в их компиляторах разрешаются все эти "UB". Но это уже по сути бюрократия, мне не особо интересная:)
Я хочу сказать, что в интернете уже столько развенчаний мифов о UB с наглядными примерами того, как UB стреляет по ногам, что нужно быть очень высокого мнения о себе чтобы пускаться в рассуждения о детерминизме компилятора.
Сначала покритикую, а потом отвечу по существу.
В какую же хрень превратился современный С++ :(
С подключением! Простите за резкость, но вы явно не программист на C++. Вы из какого языка пришли в статью о C++? Из C? (Это риторический вопрос)
Такой ситаксис появился ещё в 2011 году.
не remove, а инверсия
Что, простите? Вы не знаете как XOR работает?
Ну а вообще есть битовые поля.
Они от пачки булевых не отличаются. Использовать union при этом - это намеренно тащить UB в код. Спасибо. Не надо Не в каждой компании code review пройдёт
Что, простите? Вы не знаете как XOR работает?
В том-то и дело, что знаем. К слову, та же самая операция XOR могла бы и в методе add применяться, но там вы "почему-то" использовали OR.
С подключением! Простите за резкость, но вы явно не программист на C++. Вы из какого языка пришли в статью о C++? Из C? (Это риторический вопрос)Такой ситаксис появился ещё в 2011 году.
Что, простите? Вы не знаете как XOR работает?
Как удачно совпало:) В вашем коде полно шаблонов (да еще и вариадических), constexpr'ов, static_cast'ов и прочего, все написано в лучших традициях "современного С++". Но все эти навороты вам не помогли - вы допустили ошибку, применив XOR, который инвертирует значение на противоположное вместо того, чтобы его обнулять, что ожидается по смыслу от функции с именем remove().
Пройдите в википедию (хотя бы) для начала, и посмотрите, как работает XOR, чтобы не писать чуши.
Я прекрасно знаю как xor работает, постоянно его использую:)
X Y X^Y
0 0 0
0 1 1
1 0 1
1 1 0Таким образом, если вы случайно вызовете метод remove() для некоторого бита два раза (в общем случае четное число раз), то результат будет такой же, как если бы вы его вообще не вызывали. Что принципиально отличается от add(), который можно вызывать какое угодно число раз - бит всегда будет устанавливаться в единицу. Также работа remove() будет зависеть от текущего состояния бита, а работа add() - нет.
Если вы знаете как он работает, то почему вы его «инверсией» называете? Когда он «инверсией» в полном смысле не является. То, что где-то его называют инвертирование по маске, не делает его инверсией. А остальное я не буду комментировать. Напридумывали сами себе проблем, и пытаетесь меня ими попрекать. Ваши позицию я понял
Потому что, если зафиксировать второй операнд, таблица истинности превращается в вот такую:
X 1 X^1
0 1 1
1 1 0
Что это если не инверсия?
Более того, в той же алгебре Жегалкина именно так инверсия и выражается.
Что это если не инверсия?
То, что XOR - это разница. Я удивлён комментариями. Я принимаю критику. Я всегда анализирую полученную обратную связь, когда она по существу. Но люди на полном серьёзе пишут чушь, и ещё и меня пытаются в эту чушь загнать. Я, увы, это принять не могу
Вы можете хоть сто раз назвать всё чушью, но поведение вашего метода remove от этого соответствовать его названию не станет.
Давайте зафиксируем. Я знаю и принимаю факт, что идиоматический x&~y - это удаление. Но моя логика в том, что здесь уже две операции, против одной в моём варианте. Удалит ли XOR Условие, если это необходимо, после проверки на наличие? Удалит. XOR вернёт разницу. Был ли мой выбор оператора верным? Зависит от стороны спора. Дальше нет смысла дискутировать. Надеюсь читатель статьи прочитает комментарии, и делает свой выбор оператора для удаления
Что-то вы странно операции считаете, у вас там условный оператор стоит, который сам по себе дороже побитовой инверсии.
Условный оператор там стоит для проверки. Об этом написано в статье. Мне она была нужна. Потому я её там и написал
И снова эту проверку можно и без него сделать.
Условный оператор там стоит для проверки. Об этом написано в статье. Мне она была нужна.
А не потому ли условный оператор с вызовом has() понадобился, что remove() не делает то, что в ее названии указано? А делает ту самую "инверсию" x^y вместо логичного и правильного x&~y.
Да и странно считать количество логических операций при этом используя условный оператор и вызов метода. Ясно, что там будет инлайн, да всякие оптимизации компилятора, но вы об этом не говорите.
Да и откуда взялись вообще ваши рассказы в комментариях про производительность? В тексте статьи об этом условии ни слова. И нужно ли вообще беспокоиться о производительности и колличестве логических опреторов в описанной вами задаче? Поэтому повторюсь - чем вас std::bitset не устраивает?
У вас метод reach(), который вызывает remove(), работает правильно только потому, что перед вызовом remove() вы делаете проверку has() на нулевое значение проверяемого бита и не вызываете remove() в этом случае если 0. А в случае когда бит равен 1, remove() его просто инвертирует в 0, как вам и пытаются тут показать, что XOR в remove() неуместен, выпполняет инверсию, и делает не то, что вы ожидаете, хоть и все работает в итоге правильно, но до тех пор, пока кто-то не удалил проверку на 0.
Пройдите в википедию (хотя бы) для начала, и посмотрите, как работает XOR, чтобы не писать чуши.
Что-то инверсные условия выглядят каким-то переусложнением. Как и названия методов reached, lose, isReached - это ж головоломка какая-то. Да и разделение методов на публичные и приватные тут нафиг не нужно...
Можно же проще:
public:
void set(EnumType flags) noexcept
{
value |= static_cast<EnumUnderlyingType>(flags);
}
void reset(EnumType set) noexcept
{
value &= ~static_cast<EnumUnderlyingType>(condition);
}
bool has(EnumType flags) const noexcept
{
return (value & static_cast<EnumUnderlyingType>(flags)) == static_cast<EnumUnderlyingType>(flags);
}
bool has_any(EnumType flags) const noexcept
{
return (value & static_cast<EnumUnderlyingType>(flags));
}
Что-то я изначально хотел длинный комментарий, разбирая ваш. А сейчас посидел и принял, что пусть так. Для вас это и переусложнение, и лишние приватные методы. К решению задачи можно подойти разными путями. Я подошёл таким, который описал в статье. А ваш комментарий - придирки по пустякам
А ваш комментарий - придирки по пустякам
Тогда как со стороны он выглядит конструктивной критикой. И решение ваше простым не является, и по поводу именования методов полностью согласен с предыдущим оратором.
ЗЫ. С собственными решениями всегда так. По началу они нравятся, хочется поделиться с народом удовольствием о того, что удалось придумать, сделать и заставить работать. Однако, не редко, годика эдак через три-четыре-пять, сам уже бываешь не рад и осознаешь, что нужно было делать проще.
Тогда как со стороны он выглядит конструктивной критикой
Я на самом деле принимаю эту критику. Только есть одна проблема. Я изначально в представленном коде написал комментарий о том, что приватные методы лишь для наглядности. Если то, кто скопирует код, и переделает это под свои критерии, то я не буду этому препятствовать. Он сам у себя дома хозяин. С моей стороны я постарался сделать наглядный пример. Но уже у второго комментатора придирка именно к части битовых операций. И оба отбрасывают возможность проверки. Опять же пусть так. Им не нужно. Но нужно было мне. И именно так и написан мной код
upd.
И придирка к наименованию. Что значит set? Оно снимает или устанавливает? А reset?
isActive.set(Condition::INITIALIZED);
isActive.reset(Condition::INITIALIZED);
Особенно в свете вашего же тезиса о годах спустя
set устанавливает, reset сбрасывает.
Это повсеместная терминология, Навскидку: RS-триггеры, объект Event в WinAPI, тот же bitset из стандартной библиотеки С++ - все используют set и reset именно так.
Семантика слова reset подразумевает установку первоначального значения. А какое первоначальное значение у бита в переменной представленного мой контейнера? С учётом моего примера инициализации, то бит должен быть 1. Но ваша функция ставит его в 0. Я в этом не вижу очевидного поведения
Семантика слова reset подразумевает установку первоначального значения.
Как бы reset означает сброс того, что было установлено раньше. Будет ли результат reset-а начальным значением или нет -- это уже от предметной области зависит.
Об этом я уже говорил, нет никакого смысла в хранении инвертированных флагов. Начальное значение - ноль.
Особенно в свете вашего же тезиса о годах спустя
Вновь поддержу предыдущего оратора. set устанавливает, reset сбрасывает.
Т.е. если кто-то вызывал:
bits_collection.set(Condition::INITIALIZED);
то я ожидаю, что условная функция проверки is_set вернет true:
bits_collection.set(Condition::INITIALIZED);
assert(bits_collection.is_set(Condition::INITIALIZED);
Тогда как reset ведет к тому, что is_set вернет false:
bits_collection.reset(Condition::INITIALIZED);
assert(!bits_collection.is_set(Condition::INITIALIZED);
Как признак set/unset будет хранится внутри -- это безразницы, хоть ноликом, хоть единичкой.
PS. И да, не стоит в C++ давать идентификаторам названия в ИСКЛЮЧИТЕЛЬНО_ВЕРХНЕМ_РЕГИСТРЕ. Прямой путь к проблемам при интеграции с C-шным кодом.
И да, не стоит в C++ давать идентификаторам названия в ИСКЛЮЧИТЕЛЬНО_ВЕРХНЕМ_РЕГИСТРЕ
Это зависит от гайду по стилю коду принятого в компании, а не от ваших личных предпочтений. Названия перечислений и константные значения, на сколько я помню, принято указывать заглавными буквами. Очередная придирка по пустяку
а не от ваших личных предпочтений
Если бы.
Названия в CAPS_LOCK-е принято давать в C-шных макросах.
Поэтому если вы у себя в проекте подключаете заголовок из чисто-сишной либы, то к вам могут прилететь макросы, о которых вы даже не подозреваете. И ваш код начнет вести себя непредсказуемо (хорошо хоть, что в основном во время компиляции).
Я так когда-то наступил на грабли с названием LOG_ERR (или чем-то подобным).
Названия перечислений и константные значения, на сколько я помню, принято указывать заглавными буквами.
В Java :)
Очередная придирка по пустяку
Что-то мне подсказывает, что вы еще очень молоды.
С константами в верхнем регистре я беру свои слова назад. Вот если бы вы мне ссылку на гайдлайн дали, то я бы не стал писать комментарий выше. Я не разработчик на Java, никогда им не был. И вообще Java не люблю. Потому не знаю про их стиль кода. Константы и перечисления с заглавными буквами, меня научили писать ещё очень давно. И я до сих пор так пишу, и в некоторых компаниях, где работал, принято так же. И сейчас я понимаю, что всё это время писал не правильно, если идти по гайдлайну по ссылке
Под впечатлением от решения подобной проблемы на стороне свифта, с помощью magic_enum был написан простой враппер над std::bitset, идея простая, в std::bitset поменять тип аргумента с std::size_t на enum class. Размер std::bitset можно вычислить с помощью magic_enum.
Из плюсов тут то, что нет требования к значениям enum, что они должны быть степенями двойки, но есть требования этим значениям вообще ничего не присваивать, чтобы они определялись автоматически, иначе беда. Ждем рефлексию чтоб такое можно было делать более безопасно.
Hidden text
template <class Enum, std::size_t Size = magic_enum::enum_count<Enum>()>
class OptionSet {
std::bitset<Size> _bits;
public:
OptionSet() = default;
OptionSet(Enum e) {
set(e);
}
OptionSet(std::initializer_list<Enum> list) {
for (auto e : list) {
set(e);
}
}
constexpr typename std::bitset<Size>::reference operator[](Enum e) {
return _bits[std::to_underlying(e)];
}
constexpr bool operator[](Enum e) const {
return _bits.test(std::to_underlying(e));
}
// Далее врапим все методы std::bitset
};
Контейнер ConditionalBitset — небольшое хранилище для условий выполнения