К вопросу о стиле программирования

«Если рассматривать шкалу духовных ценностей по нисходящей, существуют
Вещи В Порядке Вещей, существуют
Вещи Неприятные, Но В Принципе Допустимые, и существуют
Вещи, Которые Терпеть Никак Нельзя». — Мидянин

Как говорил небезызвестный классик «Не могу молчать».
Недавно смотрел исходные тексты на сайте довольно-таки известного производителя и увидел следующий код

*(unsigned int *)0xf80ff000 &= 0xffffefff;

Не надеясь, что эти заметки прочтут в далекой «Индии» (смотри примечание ниже) (складывается ощущение, что они и читать то не умеют), тем не менее хотел бы предостеречь молодых инженеров — так делать НЕЛЬЗЯ.

Примечание от сегодняшнего дня — набросал этот пост больше месяца назад, все руки не доходили довести его до приемлемого вида, так что источник указать не могу, ну да Инет Вам в помощь — подобного кода там чуть меньше, чем очень много.

Существует море литературы, в которой написано, как делать можно и нужно (ну так кто же ее читает, там много букв), я настоятельно рекомендую стандарты MISRA, но подобные программные конструкции там даже не рассматриваются, поскольку они лежат по ту сторону границы, отделяющую добро от зла. Но, поскольку такие конструкции все еще встречаются, и их могут увидеть дети (а правильная цензура Инета все еще не налажена), наверное, придется еще раз объяснить, почему так делать нельзя.

Для начала поймем, что же хотели сделать этим фрагментом кода наши далекие индийские коллеги (автор нисколько не шовинист и готов допустить, что код написан китайскими, американскими либо русскими программистами, просто индусский код стал нарицательным термином). Очевидно, что они хотели сбросить бит номер 12 (считая с 0) в слове с шестнадцатеричным адресом f80ff000. Скорее всего, там расположен регистр какого-то внешнего устройства, и, скорее всего, они сбрасывают бит готовности, что, впрочем, совершенно неважно. Итак, сама по себе операция не настолько ужасна, чтобы подвигнуть на автора на пространный пост, и, тем не менее, начнем разбор полетов.

Прежде всего, в одной строке использовано сразу 2 магические константы — для адреса и для данных. Мне трудно поверить, что больше нигде в программе не будет ни одного обращения к этому регистру (так оно и есть, подобные строки можно увидеть далее по исходному коду) и поэтому использование магической константы ни имеет не малейшего оправдания. Как известно, #define именно для этого и придуман, и хотя в последнее время я видел немало убедительных материалов о необходимости использования констант из списка enum вместо него, даже самое неудачное применение дефайна лучше, чем то, что мы видим.

Далее, данные использованы не просто в виде магической константы, но это явно константа преобразованная, то есть инверсия от битовой маски. Единственный аргумент, который я мог бы изобрести в пользу подобного решения, это снижение времени компиляции, но мне данное предположение представляется настолько исчезающе ничтожной мотивировкой, что приведено исключительно в теоретических целях (ну и чтобы почеркнуть объективность автора и его снисходительность к человеческим слабостям).

А ну да, я забыл еще один аргумент сторонников констант — именованные константы захламляют таблицу имен компилятора и увеличивают требования к размеру оперативной памяти во время компиляции. Я не смеюсь, я действительно слышал такой аргумент от вроде как вменяемого человека, потом выяснилось, что это был тонкий троллинг (но десяток неприятных секунд мне это принесло — неужели я мог НАСТОЛЬКО в человеке ошибаться).

На этом прекращаем попытки оправдать авторов подобного кода (да я не особо то и пытался) и методом последовательных приближений пойдем к нормальному коду.

#define CsrReg *(unsigned int *)0xf80ff000 // регистр состояний, стр. 127 
#define CsrRegReady 0x1000 // бит готовности 
 CsrReg &= ~CsrRegReady;

уже выглядит лучше, а мы еще даже не в середине пути, хотя некоторые на этом и остановятся. Что не так в этом фрагменте? Прежде всего прямая операция инверсии, если Вы счастливый человек, то Вы никогда ее не пропустите при написании. Я не столь счастлив (может быть, не столь внимателен и собран, как Вы), поэтому у меня бывало пропустить тильду, а потом бегать по коду в поисках места ошибки.

Поэтому макросы наше все (ну или inline функции, если Вы перешли на плюсы)

#define RegBitClr(REG,MASK) (REG) &= ~(MASK)
 RegBitClr(CsrReg,CsrRegReady);

(я знаю, что имена макросов надо писать большими буквами, и даже понимаю аргументы в пользу данного утверждения, но не принимаю их — я художник, я так вижу). Для того, чтобы понять, что тут не так, хорошо бы посмотреть порожденный код с включенной оптимизацией для следующего фрагмента

 RegBitClr(CsrReg,CsrRegReady);
 RegBitClr(CsrReg,~CsrRegReady); // здесь тильда поставлена специально

Я его приводить не буду, Вам остается только поверить мне на слово, что первая строка игнорируется, а вторая порождает код, эквивалентный выражению CSrReg=0. И действительно, с точки зрения компилятора, в первой строке мы сбросили все биты, кроме 12, а во второй сбросили и его, то есть будет ноль.

Но на самом то деле это совершенно не очевидно, поскольку мы имеем дело с регистром, то есть нам нужно прямое указание компилятору на изменчивый характер переменой и мы получаем

#define CsrAdr (unsigned int *)0xf80ff000
volatile unsigned int * const CsrReg = CsrAdr;
#define RegBitClr(REG,MASK) *(REG) &= ~(MASK)
 RegBitClr(CsrReg,CsrRegReady);

Вот тут уже все почти хорошо, обратим внимание, что в этом варианте мы уже не можем написать

 RegBitClr(CsrRegReady,CsrReg);

, поскольку компилятор нас обругает. Конечно, мы и не должны так писать, это неправильное выражение, но этот пост предназначен нормальным людям, которым свойственно ошибаться, полубоги от программирования, которые никогда не ошибаются, могут вообще делать что им заблагорассудится и не думать о таких скучных вещах, как хороший стиль и проверки типов компилятором. («Только посредственности нуждаются в порядке, Гений властвует на Хаосом.»)

Что осталось не слишком удачным? Возможность написать что-нибудь типа

 RegBitClr(CsrReg,3);

, мы ведь договорились, что должны избегать магических констант, и лучше бы, если бы компилятор нас одергивал, но, к сожалению, в рамках С это недостижимо, вспомним один из недостатков макросов по отношению к функциям — они не проверяют параметры. Перейти же к настоящим функция мы не можем, поскольку inline не является в C директивой компилятора (ну, по крайней мере, так в моей версии IAR, хотя я вроде это победил при помощи прагмы — костыли, повсюду костыли), а вызывать настоящую функцию для сброса бита будет весьма накладно, иначе бы

typedef volatile unsigned int * const Reg_t;
enum CsrRegBits {CsrRegReady=0x1000,CsrRegDone=0x100};
inline void RegBitClr(Reg_t Reg, const enum CsrBits Mask) {
 *Reg &= ~Mask;
};  
 RegBitClr(CsrReg,CsrRegReady);
 RegBitClr(CsrReg,~CsrRegReady); // тут и 
 RegBitClr(CsrReg,3);  // тут предупреждения компилятора

было бы почти идеальным решением.

Правда, нельзя написать и

 RegBitClrf(CsrReg,CsrRegReady | CsrRegDone); // тут тоже предупреждение компилятора
 RegBitClrf(CsrReg,(enum CsrRegBits)(CsrRegReady | CsrRegDone)); // а вот так можно
 

Конечно, последняя строка позволяет создать и неприемлемые выражения, но, по крайней мере, Вы явно указали, что ознакомлены с длиной веревки. Единственно, что порекомендую для такого решения, по крайней мере для IAR, поставить флажок против «Treat all warnings as errors» чтобы сделать контроль жестким.

Ну и заключение — мы пока что рассматривали только однозадачную непрерываемую программу. Представим, что Вам надо учесть возможность прерываний, я то это сделаю путем добавления двух строчек в текст макроса либо функции, а вот что будут делать авторы исхлжного фрагмента, мне даже и представить трудно.

Автор: GarryC

Источник ^[1]