Если не strcmp...

Опять встретилась проблема в коде. Не то, чтобы очень уж страшная проблема, некоторые даже думают что тем самым они наоборот вносят ясность в код. Но я, как обычно, против.

Давайте посмотрим на следующий код, который я для примера выдрал из ядра linux:

if (!strncmp(name, p, k) && p[k] == '=') {
 	p += k + 1;
	...

Семь раз отмерь...

Интересно, откуда пошла поговорка? Но речь не об этом.

Если жить в соответствии с дао, я имею ввиду TDD и рефакторинг, то методы надо делать меньше. Как сказал Роберт Мартин - 'нет, методы надо делать еще меньше'. И классы в соответствии с SRP надо делать меньше.

Я никогда не встречал проектов, которые бы разрабатывались в полном соответствии с этой концепцией, но чувствую проблемы. Проблемы заключаются в том, что количество таких объектов может уйти далеко за рамки 7+-2, которые человек в состоянии эффективно анализировать.

Триггеры perforce

Наша разработка ведется очень демократично. Мы пишем под FreeBSD (исторически и лицензионно сложилось), используем кодировку koi8-r (тоже исторически сложилось, FreeBSD не очень то дружит с UTF-8 и по сей день).

Но в то же время мы не навязываем разработчикам никаких условий... Каждый работает так, как ему комфортнее. Жить под FreeBSD - я не пожелаю и врагу (ну вы уже знаете, как я ее не люблю :) ). Наши разработчики пользуются своими любимыми редакторами, сидят в своих любимых системах с разными локалями и интерпретациями концов строк.

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

Больше предупреждений, хороший и разных

Хорошая практика программирования рекомендует включать максимум предупреждений компилятора и добиваться того, чтобы их не возникало в процессе компиляции. Заставить один C++ проект компилироваться без предупреждений достаточно легко. Но если в рамках одного проекта используются модули на C/C++ - все становится значительно интереснее.

Есть предупреждения, применимые только для C++ (например: -Weffc++, -Woverloaded-virtual, -Wold-style-cast). Я не пробовал включать их для си, могу предположить, что последует ругань и правильно. А поскольку для C они не имеют смысла - в данном контексте они не интересны.

Для обоих языков в первом приближении я отобрал три варнинга: -Wsign-conversion -Wconversion -pedantic. Остановимся на них поподробнее.

Начать стоит с того, что в си enum - всегда int.

enum {
 TEST1 = 0x100000000ULL,
 TEST2 = 0xffffffffU,
 TEST3 = 0x7fffffff
};
$ gcc -c -std=c99 -pedantic test.c
test.c:2: предупреждение: в ISO C значения перечислимого типа ограничены диапазоном типа ‘int’
test.c:3: предупреждение: в ISO C значения перечислимого типа ограничены диапазоном типа ‘int’

Причем это справедливо и для amd64, в этом легко убедиться например так:

BOOST_STATIC_ASSERT(sizeof(int) == 4);

В то же время в C++ с enum не возникает никаких проблем. В C++ тип enum есть собственно enum, даже знаковость которого определяется позже. Единственный возникающий в данной ситуации момент - стандарт C++ 99 года не любит определений ULL, считая их C99 long long integer constant, Для C++ даже U писать не обязательно, главное, чтобы значение могло уместиться в long, то есть быть не больше 32-64 бит в зависимости от типа платформы.

Это наводит на мысль, что переносимое приложение не должно использовать в enum значения больше 32 бит длиной.

Значение sizeof(TEST1) в С++ варьируется в зависимости от наполнения enum от 4 до 8.

Другой интересный момент, справедливый для обоих языков:

const unsigned int t1 = TEST1;
const unsigned int t2 = 0;
const unsigned int t3 = argc > 1 ? TEST1 : 0;
$ gcc -m64 -o test -std=c99 -Wsign-conversion test.c
test.c:3: предупреждение: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result

Ошибка возникает в третьей строке фрагмента, и лечится заменой 0, на 0U... Хотя тот же 0 во второй строке не вызывает проблем.

И напоследок - ошибочная ситуация в gcc:
При попытке присвоить битовому полю значение какой-то переменной практически всегда происходит ошибка, и обойти ее практически не представляется возможным.

unsigned int x = 0x7f;
struct {
 unsigned int a:7;
} t = { .a = x };
$ gcc -std=c99 -Wconversion test.c
test.c:4: предупреждение: conversion to ‘unsigned char:7’ from ‘unsigned int’ may alter its value

Не существует способа привести переменную к битовому полю.

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

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

Можно предложить борцам на чистоту кода периодически устраивать параноидальные проверки с большим количеством опций, которые, возможно, не удастся расчистить целиком. А в обычные дни обходится более скромным подмножеством опций, но при этом обязательно используя -Werror. :)

PS: Интересное на этом конечно же не заканчивается - это лишь первый взгляд. Может быть последует продолжение, если будет не лень, совсем я что-то забыл про блог.

Эффективная работа с унаследованным кодом

Вышла в свет русская версия книги Майкла К. Физерса (Michael Feathers) - "Эффективная работа с унаследованным кодом" (Working Effectively with Legacy Code). Какой я блин шустрый, успел заказать ее на озоне за 854 рубля, хотя сейчас она стоит уже 909...

Не пропустите.



PS: Только что-то Озон не торопится мне её доставлять. Уже два дня как должен был доставить.

PPS: Что-то я совсем расслабился, в блог писать некогда. :)

Хронический аудиал

Что касается женщин, то это чувство сугубо интимное...

Есть некоторые идеомы, считающиеся хорошими. Например считается правильным писать константную часть слева в условии, для того чтобы на операцию '=' вместо '==' ругался компилятор а не конечный пользователь.

if (MIN_ELEMENTS == i) ...

Я долго не мог понять почему мне это так не нравится, и наконец сформулировал для себя ответ на этот вопрос.

Писать так в частности рекоммендует Стив Макконел (Steve McConnell) в книге "Совершенный код". Там же рекоммендуется упорядочивании аргументы в соответствии со значениями на числовой прямой.

if (MIN_ELEMENTS <= i && i <= MAX_ELEMENTS) ...

Очень похожая ситуация, хотя меня она коробит меньше.

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

Может быть такая запись считается удобной и правильной для тех, кто оценивает код визуально. Что же касается меня - я код предпочитаю читать. И читается примерно следующее:

Если MIN_ELENEMTS равен...

Стоп, как может быть константное значение сегодня равно, а завтра не равно? В этом выражении как будто содержится предположение, что MIN_ELEMENTS может измениться.

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

i равно чему-то; если i равно MAX_ELEMENTS...

Все последовательно и логично. Но это если код читать... Если воспринимать код целостно, то, возможно, все воспринимается несколько иначе.

Тесты наверное врут, когда определяют меня как кинестатика-визуала. Хотя с другой стороны я люблю чтобы было красиво. Я смотрю на код и целиком. Только когда я смотрю на код - я почему-то не смотрю на идентификаторы, Свысока я разглядываю группы операторов, то есть мыслю уже на другом уровне.

С другой стороны неприятие распределения в соответствии с числовой прямой может связано со складом ума. Предполагаю, что эта запись очень близка к математической, сам то я от математики далек. Может быть людям с математическим складом ума она кажется более естественной чем мне, гуманитарию.

Я конечно такой человек, что никогда не бываю полностью довольным существующим положением вещей. Но мне бы очень не понравилось, если бы подобные идеомы были бы внесены в соглашение о кодировании. Слишком индивидуальные это вещи.

PS: Новогодние праздники сильно выбивают из колеи. Ленишься и ничего не хочется делать. Приходится себя заставлять, ну хоть сегодня напиши хоть что нибудь. :)

О практической пользе константных параметров...

Практики хорошего программирования рекоммендуют никогда не менять параметры функций в теле. Но чрезмерное применение const так загромождает прототип функции, не хочется, но альтернативы этому нету. const'ом метод не испортишь :) и параметр, как показывает практика - тоже.

Не далее как вчера описывал точки входов системных вызовов в ядро. Логика ассемблерной прослойки проста - сохранить все кроме eax, вызвать высокоуровневый метод-обработчик и корректно же выйти. Но это выливается примерно в такой код:

KernelWait:
       pushfl
       pushl %ebx
       pushl %ecx
       pushl %edx
       pushl %esi
       pushl %edi
       pushl %ebp

       pushl %ds
       pushl %es
       movl $KERNEL_DATA_SELECTOR, %ebx
       movw %bx, %ds
       movw %bx, %es

       pushl %edx
       pushl %ecx
       pushl %ebx
       pushl %eax
       call StubWait
       add $16, %esp

       popl %ds
       popl %es

       popl %ebp
       popl %edi
       popl %esi
       popl %edx
       popl %ecx
       popl %ebx
       popfl

       iret

Глядя на эту функцию часть кода может показаться лишней. Вполне логично было бы написать так:

KernelWait:
        ...
       pushl %edx
       pushl %ecx
       pushl %ebx
       pushl %eax
       call StubWait
       add $4, %esp
       popl %edx
       popl %ecx
       popl %ebx
        ...
       iret

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

int StubWait (int a, int b, int c, int d) 
{
       d += 10;

Очень плохо! Разумным способом предотвращения данной ситуации является описание всех параметров константными.

int StubWait (const int a, const int b, const int c, const int d) 
{
        int dnew = d + 10;

Так значительно лучше и не грозит проблемами. Кроме того думаю, что привычка объявлять все методы функций константными даст компилятору больший простор для оптимизации. Это предположение, но компиляторы сейчас умные, обявление константости в следующем случае:

static const
struct console StubXGAConsole = {
       .putc = StubXGAPutC,
       .getc = StubXGAGetC,
};

static const
struct console *sConsole = &StubXGAConsole;

позволило компилятору, при отсутствии явной модификации sConsole, вообще атрофировать эту переменную, не смотря на то, что сам по себе sConsole модифицируемый.

PS: А экономия на push/pop очень напоминает преждевременную оптимизацию, к тому же нелогично после вызова функции пупать параметры обратно, поэтому лучше я воздержусь от такого вида оптимизации. Тем более что я использую единственный макрос для всех 8 системных вызовов.