LINUX.ORG.RU

Werror - контроль качества или занудство?

 ,


0

4

Все, кто занимается низкоуровневым жонглированием байтами в реалых промышленных условиях (с кучей поставщиков со всего мира, а не в уютненьком молодёжном стартапе где-нибудь в Саннивейл), понимают, что C’шка с нами надолго, лет на 20, если не больше. И поэтому возникает желание максимально использовать существующие технологии для обеспечения качества кода.

Например, Werror (и то, что к нему полагается в виде -Wall, -Wextra и проч.). Но возникают такие ситуации, как например с «целочисленным повышением» и последующим сравнением с разным знаком. Например:

const unsigned x = 12;
unsigned char y;
unsigned char z;
... // что-то кладём в y и z
if (x < (y*z))
{
   // тра-ля-ля
}

И y*z превращаются («брюки превращаются, превращаются брюки…») в элегантный int, и вылезает предупреждение о различной знаковости, как бы совершенно на ровном месте. Т.е. теперь, чтобы ублажить компилятор, надо дополнительно, например, явно кастануть x к int’у. Т.е., код уже на пределе читаемости (выше пример - это сильное упрощение возможной реальной ситуации), и тут мы ещё добавляем вовсе не интуитивный (int).

И возникает вопрос: а стоит ли овчинка (-Werror и ко.) выделки? Я сейчас, очевидно, не имею в виду код наивысшей критичности, а такой, который при случае можно просто неспеша поправить, в конце рабочего дня, с нулевыми последствиями для пользователей, окружающей среды и т.п.

P.S. кстати, поскольку в расте тоже есть беззнаковые типы, там тоже нечто подобное должно быть, или как?

UPD: в примере, в нагрузку к умножению надо ещё добавить сложение с ещё одним unsigned char.

★★★★★

Последнее исправление: seiken (всего исправлений: 1)

  1. Ты должен компилировать код по возможности с максимальным количеством включенных дианостик. Потому что базовый синтаксис Си - картошка. И без диагностик компилятор молча проглатывает то, что должно быть ошибками согласно любому здравому смыслу.
  2. Выборочно выключать надо те диагностики, которые генерируют большое количество бесполезных предупреждений и не указывают на реальные проблемы в коде. Например, в коде, над которым я сейчас работаю - это -Wno-unused-parameter -Wno-missing-field-initializers.
  3. В идеале, с настройками, заданными в п 1 и 2, код должен компилироваться с нулём диагностик на машине разработчика и на сборочной ферме. Это нужно, чтобы сразу, как только появляется новая диагностика при компиляции, она была тут же заметна, а не утонула в море других.
  4. Кроме этого, стоит периодически проверять код при помощи статических анализаторов, встроенных в gcc, в clang и при помощи cppcheck. Разумеется, если эти инструменты актуальны для целевой платформы. Они нередко дают false positives, так что требуют внимательности - где он по делу возмущается, а где нет.
  5. Чтобы энфорсить п. 3, на девелоперской машине сборки выполняются с -Werror. Чтобы меньше руки чесались коммитить код с диагностиками в git.
  6. Если продукт поставлятся в виде исходного кода, то в релизном тарболе -Werror по умолчанию должен быть выключен. Это необходимо потому, что пользователь может компилировать код другой версией компилятора или с другими версиями зависимостей, и на его машине могут отображаться дополнительные диагностики. Они не должны мешать компиляции.
wandrien ★★
()
Последнее исправление: wandrien (всего исправлений: 4)
Ответ на: комментарий от wandrien

В нашем случае нам даётся SDK с исходниками, которые мы адаптируем под наши нужды. Код даётся, который у поставщика собирается вообще без каких либо флагов о ворнингах.

И хрень в том, что до меня уже синтегрировали в продукт такую версию исходников.

И если в новой версии SDK появятся мои правки в самом SDK, пусть даже чтобы все ворнинги исчезли, появятся вопросы. А пулл-реквест принимают люди, которые професионально ближе к железу, чем к корректности ПО. Как-то так.

seiken ★★★★★
() автор топика
Ответ на: комментарий от wandrien

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

seiken ★★★★★
() автор топика
Ответ на: комментарий от wandrien

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

Позволю процитировать Кернигана и Ричи: программист сам должен знать, что делает, и поэтому язык Си не мешает ему и позволяет думать самостоятельно.

Ну или как-то так, но суть та же. Там в самом начале еще :)

Zhbert ★★★★★
()
Ответ на: комментарий от Zhbert

язык Си не мешает ему и позволяет думать самостоятельно.

Классика: https://github.com/sde-gui/libsmfm-core/commit/37a5cdce8eba7597e5570df7181ae2e1218fe985

Думал самостоятельно vs прочитал диагностику GCC. )

wandrien ★★
()
Ответ на: комментарий от Zhbert

программист сам должен знать, что делает

В теории должен, по факту - пук-среньк переполнение чуть менее чем во всех сишных программах. Как-то не задалось у прогеров с пониманием своих действий, кароч.

alex1101
()

-Werror хорош только на машине разработчика и ci. Он не должен включаться принудительно (разве что -Werror для отдельных ворнингов).
Иначе работающий у разарботчика код не соберётся где-нибудь с другим компилятором

mittorn ★★★★★
()
Ответ на: комментарий от mittorn

Аппаратных архитектур всего две, AMD64 и ARM64, «базовые» варианты без SIMD, без Thumb и проч.

И тогда вопрос стоит так, что нужно иметь такой исходный код, который соберётся и vs компилятором, и gcc, как с Werror,Wall,Wextra, так и без оных.

Разумеется, использование ОС-специфичного API для ввода/вывода остаётся за рамками обсуждения, речь о общей части кода.

seiken ★★★★★
() автор топика

а давайте мерятся размером варнингов!

вот такое у меня в текущем проекте, правда это С++:

-Wall -Wextra -pedantic -pedantic-errors -Wredundant-decls -Wcast-align -Wcast-qual -Wundef -Wfloat-equal -Wunreachable-code -Wmissing-include-dirs -Wnoexcept -Wpointer-arith -Wwrite-strings -Wlogical-op -Wlogical-not-parentheses -Wbool-compare -Wint-in-bool-context -Wmisleading-indentation -Wswitch -Wswitch-default -Wswitch-bool -Wsign-promo -Wnon-virtual-dtor -Wctor-dtor-privacy
alysnix ★★★
()
Ответ на: комментарий от seiken

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

mittorn ★★★★★
()

тут вопрос - а почему произведение двух unsigned стало знаковым? у проца же две команды умножения - знаковое и беззнаковое. то есть он применил знаковое умножение что ли? или беззнаковое, и посчитал результат знаковым? ерунда какая-то

alysnix ★★★
()
Ответ на: комментарий от alysnix

Повышает разрядность чтобы влез результат, ближайшее это short int. В таких случаях надо не умножать, а делить, чтобы остаться в той же разрядной сетке или индивидуально ограничить каждый множитель, но если есть возможность повысить разрядность, то лучше в явном виде ее повысить, но это не всегда возможно, например, для size_t.

soomrack ★★★★
()
Ответ на: комментарий от alysnix

Оно и не стало, подозреваю что ТС привёл не тот пример. Запутаться в implicit conversions (и не только в этом) обычное дело, поэтому включение диагностик/варнигов/санитайзеров/линтеров в общем случае лишним не будет.

anonymous
()
Ответ на: комментарий от seiken

Ты не можешь исключить то, что появится в дефолтных ворнингах в будущем. Так же есть неотключаемые ворнинги. Которые с твоим -Werror приведут к невозмрдности сборки.
Зато можно указать -Werror= и указать то, что действительно нужно проверять в этом проекте. Да, список может быть большим, но в лимит по аргументам влезет

mittorn ★★★★★
()
Ответ на: комментарий от wandrien

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

а стандарт расширяет в знаковый всегда по сути.

alysnix ★★★
()
Ответ на: комментарий от seiken

Чтобы как минимум было видно, в том числе и тебе самому, что ты понимаешь, что ты здесь делаешь и избежать возможных ошибок на ровном месте.

Ошибки связанные с преобразованием беззнакового к знаковому и наоборот с переполнением чего-нибудь - одна из частых ошибок.

grem ★★★★★
()
Последнее исправление: grem (всего исправлений: 1)
Ответ на: комментарий от Zhbert

Позволю процитировать Кернигана и Ричи: программист сам должен знать, что делает, и поэтому язык Си не мешает ему и позволяет думать самостоятельно.

Как показывает опыт, большая часть программистов на C вообще не думают. Впрочем, судя по получившемуся язычку, в способностях товарищей К. и Р. я тоже сомневаюсь. Говнина же сраная!

hateyoufeel ★★★★★
()

Я напомню что all/extra/pedantic не включают все диагностики.

Лучше всего пройтись по документации GCC и потихоньку форсировать в error самые полезные диагностики. Так можно сформировать список диагностик которые упрощают жизнь, при этом сильно не флудят как например из категории pedantic и extra.

a1ba
()

С одной стороны не встречался с ситуацией, чтобы эти предупреждения находили баг в моём коде.

С другой стороны всегда включаю все предупреждения и легко настраиваемые линтеры.

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

vbr ★★★
()
Ответ на: комментарий от seiken

Мой вопрос - почему нет варнинга в моём/твоем примере. Умные регистранты обьяснили мне, что y*z должно привестись к int и гугл тоже настаивает на этом, но варнинга нет. И теперь я не пойму почему варнинга нет.

anonymous
()
Ответ на: комментарий от anonymous

Потому, что тут нет проблемы: a < b * c, int справа, char слева, тут все однозначно, сюрпризов быть не может.

А вот если a < b * c + d, то тут уже могут быть проблемы, т.к. int плюс char возможно это не то, что хотел автор, но тут проблема минимальна.

А вот если сделать a = b * c, то тут проблема уже серьезная, т.к. вместо 256 может получиться 0.

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

soomrack ★★★★
()
Последнее исправление: soomrack (всего исправлений: 1)
Ответ на: комментарий от vbr

С одной стороны не встречался с ситуацией, чтобы эти предупреждения находили баг в моём коде.

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

soomrack ★★★★
()
Ответ на: комментарий от soomrack

Потому, что тут нет проблемы: a < b * c, int справа, char слева, тут все однозначно, сюрпризов быть не может.

Если слева от ‘<’, то там unsigned int:

    const unsigned x = 12;
    unsigned char y = 123;
    unsigned char z = 255;
    if (x < (y*z))

если сделаю так, варнинг будет:

    const unsigned x = 12;
    char y = 123;
    char z = 255;
    if (x < (y*z))

так тоже будет:

   const unsigned x = 12;
   int y = 2;
   if (x < y)

собираю gcc или clang с `` -Wall -Wextra -pedantic -O0

anonymous
()
Ответ на: комментарий от anonymous

В данном случае приводится к «unsigned int», можешь попробовать прибавить 1 и получить варнинг: a < b * b + 1;

видимо считается, что b*b может не поместиться в int и соотв. тогда все приводится к unsigned int.

soomrack ★★★★
()
Последнее исправление: soomrack (всего исправлений: 2)
Ответ на: комментарий от vbr

С одной стороны не встречался с ситуацией, чтобы эти предупреждения находили баг в моём коде.

Они не обязательно находят баг в текущем коде. Они обнажают те места кода, выполнение которых качественно меняется, когда (неявные) предусловия, которые соблюдались до момента Х, нарушаются из-за нового контекста использования ПО.

Например, до момента X предполагалось, что входной параметр может принять только значения [v1, v2], а в момент X кто-то подал на вход v3 за пределами данного диапазона. И не он виноват, что это привело к целочисленному переполнению, потому что тип аргумента позволяет хранить в нём v3, и нигде не задокументировано, что нельзя передавать v3.

В Аде можно создавать кастомные целочисленные типы легко и просто:

subtype Voltage is NonNegative range 0 .. 220;

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

seiken ★★★★★
() автор топика
Ответ на: комментарий от soomrack

видимо считается, что b*b может не поместиться в int и соотв. тогда все приводится к unsigned int.

Наверное. Если здесь смотреть https://en.cppreference.com/w/c/language/conversion

Integer promotion is the implicit conversion of a value of any integer type with rank less or equal to rank of int or of a bit-field of type _Bool(until C23)bool(since C23), int, signed int, unsigned int, to the value of type int or unsigned int.

If int can represent the entire range of values of the original type (or the range of values of the original bit-field), the value is converted to type int. Otherwise the value is converted to unsigned int. 

казалось бы, int хватит для произведения двух unsigned char (если в примере умножение на сложение поменять аналогичное поведение). В начале треда я был уверен, что для x*z будет unsigned (просто угадал, правила забы), сейчас правила почитал и уже не особо понимаю почему не int всё таки.

anonymous
()
Ответ на: комментарий от anonymous

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

Минимальное максимальное значение по стандарту для unsigned char – 255.

Минимальное максимальное значение для int по стандарту это 32767.

Так что все логично. uchar * uchar в int не помещается, поэтому должен быть unsigned int.

PS: с этими 64 битными системами совсем забыл, что int раньше был одно слово.

soomrack ★★★★
()
Последнее исправление: soomrack (всего исправлений: 1)