На новой работе я получаю пометки в обзорах кода для такого кода:
PowerManager::PowerManager(IMsgSender* msgSender)
: msgSender_(msgSender) { }
void PowerManager::SignalShutdown()
{
msgSender_->sendMsg("shutdown()");
}
Мне сказали, что последний метод должен читать:
void PowerManager::SignalShutdown()
{
if (msgSender_) {
msgSender_->sendMsg("shutdown()");
}
}
то есть, я должен поставить NULL
охрану вокруг msgSender_
переменной, даже если он является частным членом данных. Мне трудно удержаться от использования ругательств, чтобы описать, что я чувствую по поводу этого кусочка «мудрости». Когда я спрашиваю объяснения, я получаю кучу ужасных историй о том, как какой-то младший программист, один год, запутался в том, как должен работать класс, и случайно удалил члена, которого он не должен был иметь (и установил его NULL
потом Видимо), и все сразу взорвалось на месте сразу после выпуска продукта, и мы «научились нелегко, поверьте нам», что лучше просто NULL
проверить все .
Для меня это похоже на программирование культа грузов , простое и понятное. Несколько благонамеренных коллег искренне пытаются помочь мне «получить это» и посмотреть, как это поможет мне написать более надежный код, но ... я не могу не чувствовать, что они - те, кто этого не понимают ,
Разумно ли для стандарта кодирования требовать, чтобы каждый отдельный указатель, разыменованный в функции, проверялся в NULL
первую очередь - даже на частные элементы данных? (Примечание: чтобы дать некоторый контекст, мы делаем устройство бытовой электроники, а не систему управления воздушным движением или какой-либо другой продукт «отказ-равный-людям-умирать».)
РЕДАКТИРОВАТЬ : В приведенном выше примере msgSender_
соавтор не является обязательным. Если это когда-либо NULL
, это указывает на ошибку. Единственная причина, по которой он передается в конструктор, заключается в том, что PowerManager
его можно протестировать с помощью имитирующего IMsgSender
подкласса.
РЕЗЮМЕ : Было несколько действительно хороших ответов на этот вопрос, спасибо всем. Я принял один из @aaronps главным образом из-за его краткости. Похоже, что существует общее мнение, что:
- Обязательная
NULL
охрана для каждого разыменованного указателя является излишним, но - Вы можете обойти всю дискуссию, используя вместо этого ссылку (если возможно) или
const
указатель, и assert
операторы являются более понятной альтернативойNULL
охранникам для проверки выполнения предварительных условий функции.
источник
null
и ничего не делать - это всего лишь способ переместить ошибку вниз по ходу выполнения, что значительно усложняет поиск источника.Ответы:
Это зависит от «контракта»:
Если
PowerManager
ДОЛЖЕН иметь действительныйIMsgSender
, никогда не проверять на ноль, пусть он умрет раньше.Если, с другой стороны, он МОЖЕТ иметь
IMsgSender
, то вам нужно проверять каждый раз, когда вы используете, так просто.Заключительный комментарий по поводу истории младшего программиста, проблема на самом деле в отсутствии процедур тестирования.
источник
assert()
в каждой функции-члене, которая разыменовывает указатель, является компромиссом, который, вероятно, успокоит многих людей, но даже для меня это похоже на «умозрительную паранойю». Список вещей за пределами моей способности контролировать бесконечен, и как только я позволю себе начать беспокоиться о них, это станет действительно скользким спуском. Научившись доверять моим тестам, мои коллеги и мой отладчик помогли мне лучше спать по ночам.Я чувствую код должен читать:
Это на самом деле лучше, чем защита NULL, потому что очень ясно дает понять, что функция никогда не должна вызываться, если
msgSender_
NULL. Это также гарантирует, что вы заметите, если это произойдет.Общей «мудростью» ваших коллег будет молчаливо игнорировать эту ошибку с непредсказуемыми результатами.
В общем, ошибки легче исправить, если они обнаружены ближе к их причине. В этом примере предложенная защита NULL приведет к тому, что сообщение о выключении не будет установлено, что может привести или не привести к заметной ошибке. Вам было бы труднее работать в обратном направлении к
SignalShutdown
функции, чем если бы все приложение просто умерло, создавая удобную обратную трассировку или дамп ядра, указывающий непосредственно наSignalShutdown()
.Это немного нелогично, но сбой, как только что-то не так, делает ваш код более надежным. Это потому, что вы на самом деле находите проблемы, и, как правило, также имеете очень очевидные причины.
источник
Если msgSender никогда не должен быть
null
, вы должны поставитьnull
проверку только в конструкторе. Это также верно для любых других входных данных в классе - поставить проверку целостности в точке входа в «модуль» - класс, функцию и т. Д.Мое эмпирическое правило состоит в том, чтобы выполнять проверки целостности между границами модуля - класс в данном случае. Кроме того, класс должен быть достаточно маленьким, чтобы можно было быстро мысленно проверить целостность времени жизни членов класса, гарантируя, что такие ошибки, как неправильное удаление / нулевые назначения, будут предотвращены. Нулевая проверка, которая выполняется в коде в вашем посте, предполагает, что любое недопустимое использование фактически присваивает указателю нулевое значение, что не всегда так. Поскольку «недопустимое использование» по своей сути подразумевает, что любые предположения о обычном коде не применяются, мы не можем быть уверены, что отловим все типы ошибок указателей - например, недопустимое удаление, приращение и т. Д.
Кроме того - если вы уверены, что аргумент никогда не может быть нулевым, рассмотрите возможность использования ссылок, в зависимости от вашего использования класса. В противном случае рассмотрите возможность использования
std::unique_ptr
илиstd::shared_ptr
вместо необработанного указателя.источник
Нет, нет смысла проверять разыменование каждого указателя на наличие указателя
NULL
.Проверки нулевого указателя полезны для аргументов функции (включая аргументы конструктора), чтобы гарантировать выполнение предварительных условий или предпринять соответствующие действия, если необязательный параметр не предоставлен, и они полезны для проверки инварианта класса после того, как вы раскрыли внутреннюю часть класса. Но если единственной причиной того, что указатель стал NULL, является наличие ошибки, то нет смысла проверять. Эта ошибка могла бы так же легко установить указатель на другое недопустимое значение.
Если бы я столкнулся с ситуацией, подобной вашей, я бы задал два вопроса:
assert(msgSender_)
вместо проверки на ноль? Если вы просто установите нулевую отметку, вы, возможно, предотвратили сбой, но вы могли создать худшую ситуацию, потому что программное обеспечение продолжает исходить из того, что операция была выполнена, хотя в действительности эта операция была пропущена. Это может привести к нестабильной работе других частей программного обеспечения.источник
Этот пример, кажется, больше касается времени жизни объекта, чем того, является ли входной параметр нулевым †. Поскольку вы упоминаете, что всегда
PowerManager
должен иметь действительный аргумент, передача аргумента по указателю (что позволяет использовать нулевой указатель) кажется мне недостатком конструкции ††.IMsgSender
В подобных ситуациях я бы предпочел изменить интерфейс, чтобы требования к вызывающему абоненту обеспечивались языком:
Переписав это таким образом, вы
PowerManager
получите ссылкуIMsgSender
на все время жизни. Это, в свою очередь, также устанавливает подразумеваемое требование, котороеIMsgSender
должно существовать дольше, чемPowerManager
, и устраняет необходимость какой-либо проверки или утверждений нулевого указателя внутриPowerManager
.Вы также можете написать то же самое, используя умный указатель (через boost или c ++ 11), чтобы явно заставить
IMsgSender
жить дольше, чемPowerManager
:Этот метод предпочтителен, если возможно, что
IMsgSender
время жизни не может быть гарантированно длиннее, чем уPowerManager
(т. Е.x = new IMsgSender(); p = new PowerManager(*x);
).† Что касается указателей: безудержная проверка нуля затрудняет чтение кода и не улучшает стабильность (это улучшает внешний вид стабильности, что намного хуже).
Где-то кто-то получил адрес для памяти для хранения
IMsgSender
. Именно эта функция отвечает за обеспечение успешного распределения (проверка возвращаемых значений библиотеки или правильная обработкаstd::bad_alloc
исключений), чтобы не пропускать недопустимые указатели.Поскольку объект
PowerManager
не является владельцемIMsgSender
(он просто заимствует его на некоторое время), он не несет ответственности за выделение или уничтожение этой памяти. Это еще одна причина, почему я предпочитаю ссылку.†† Так как вы новичок в этой работе, я ожидаю, что вы взламываете существующий код. Под недостатком дизайна я подразумеваю, что недостаток заключается в коде, с которым вы работаете. Таким образом, люди, которые помечают ваш код, потому что он не проверяет нулевые указатели, действительно помечают себя для написания кода, который требует указателей :)
источник
Как и исключения, защитные условия полезны только в том случае, если вы знаете, что нужно делать для восстановления после ошибки, или если вы хотите выдать более значимое сообщение об исключении.
Глотание ошибки (будь то в качестве исключения или для проверки) - это только то, что нужно делать, когда ошибка не имеет значения. Наиболее распространенное место, где я могу видеть ошибки, которые можно проглотить, - это код регистрации ошибок - вы не хотите завершать работу приложения, потому что вы не смогли записать сообщение о состоянии.
Каждый раз, когда вызывается функция, и это не опциональное поведение, она должна давать сбои громко, а не тихо.
Отредактируйте: думая о своей истории младшего программиста, звучит так, как будто случился так, что частному члену было присвоено значение null, когда это никогда не должно было случиться. У них была проблема с неуместным написанием, и они пытаются исправить это путем проверки при чтении. Это задом наперед. К тому времени, когда вы его идентифицируете, ошибка уже произошла. Реализуемое решение для проверки кода / компилятора для этого - не защитные условия, а геттеры и сеттеры или постоянные члены.
источник
Как отметили другие, это зависит от того,
msgSender
может ли быть законным или нетNULL
. Следующее предполагает, что оно никогда не должно быть NULL.Предложенное «исправление» другими членами вашей команды нарушает принцип « Мертвые программы говорят, что нет лжи» . Ошибки действительно трудно найти как есть. Метод, который незаметно изменяет свое поведение в зависимости от более ранней проблемы, не только затрудняет поиск первой ошибки, но также добавляет и вторую собственную ошибку.
Младший сеял хаос, не проверяя на ноль. Что делать, если этот фрагмент кода наносит ущерб, продолжая работать в неопределенном состоянии (устройство включено, но программа «думает», что оно выключено)? Возможно, другая часть программы сделает что-то безопасное, только когда устройство выключено.
Любой из этих подходов позволит избежать тихих сбоев:
Используйте утверждения, как предлагается в этом ответе , но убедитесь, что они включены в производственный код. Это, конечно, может вызвать проблемы, если будут написаны другие утверждения с предположением, что они будут сняты с производства.
Брось исключение, если оно пустое.
источник
Я согласен с ловушкой нуль в конструкторе. Далее, если член объявлен в заголовке как:
Тогда указатель не может быть изменен после инициализации, поэтому, если он был хорош при построении, он будет работать в течение всего времени существования объекта, содержащего его. (Объект указал на будет не быть константными.)
источник
NULL
.const
указателем нет необходимости выходитьassert()
за пределы конструктора, поэтому этот ответ кажется даже немного лучше, чем у @Kristof Provost (который также был выдающимся, но также рассматривал вопрос несколько косвенно). Я надеюсь, что другие проголосуют за это, так как на самом деле не имеет смыслаassert
в каждом методе, когда вы можете просто сделать указательconst
.Это совершенно опасно!
Я работал под руководством старшего разработчика в кодовой базе C с самыми ненадежными «стандартами», которые настаивали на том же, чтобы вслепую проверять все указатели на ноль. В конечном итоге разработчик будет делать такие вещи:
Однажды я пытался удалить такую проверку предусловия один раз в такой функции и заменил ее на,
assert
чтобы посмотреть, что произойдет.К моему ужасу, я нашел тысячи строк кода в кодовой базе, которые передавали нулевые значения этой функции, но там, где разработчики, вероятно, смущались, обходили стороной и просто добавляли больше кода, пока все не заработало.
К моему дальнейшему ужасу, я обнаружил, что эта проблема была распространена во всех видах мест в кодовой базе, проверяющей наличие нулей. Кодовая база выросла за десятилетия, чтобы полагаться на эти проверки, чтобы иметь возможность молча нарушать даже самые явно задокументированные предварительные условия. Если убрать эти смертельные проверки в пользу
asserts
, все логические человеческие ошибки за десятилетия в кодовой базе будут раскрыты, и мы утонем в них.Потребовалось всего две, казалось бы, невинных строки кода, как это + время, и команда, чтобы в итоге замаскировать тысячу накопленных ошибок.
Это методы, которые делают ошибки зависимыми от других ошибок, чтобы программное обеспечение работало. Это кошмарный сценарий . Это также заставляет каждую логическую ошибку, связанную с нарушением таких предварительных условий, загадочным образом показывать миллионы строк кода от фактического сайта, на котором произошла ошибка, поскольку все эти нулевые проверки просто скрывают ошибку и скрывают ошибку, пока мы не достигнем места, которое забыло чтобы скрыть ошибку.
Просто слепо проверять наличие нулей в любом месте, где нулевой указатель нарушает предварительное условие, для меня - абсолютное безумие, если только ваше программное обеспечение не является настолько критически важным для сбоев утверждений и производственных сбоев, что потенциал этого сценария предпочтителен.
Так что я бы сказал, абсолютно нет. Это даже не "безопасно". Вполне может быть, что все наоборот, и замаскировать всевозможные ошибки в вашей кодовой базе, которые с годами могут привести к самым ужасным сценариям.
assert
это путь сюда. Нарушения предварительных условий не должны оставаться незамеченными, иначе закон Мерфи может легко вступить в силу.источник
assert
как механизм документирования, чтобы установить предварительные условия, а не просто способ принудительного прерывания. Но мне также нравится природа ошибки подтверждения, которая показывает вам, какая именно строка кода вызвала ошибку и не удалось выполнить условие подтверждения («документирование сбоя»). Это может пригодиться, если мы в конечном итоге используем отладочную сборку вне отладчика и застаемся врасплох.Objective-C , например, обрабатывает каждый вызов метода
nil
объекта как неиспользуемый, который оценивается как нулевое значение. У этого проектного решения в Objective-C есть некоторые преимущества по причинам, предложенным в вашем вопросе. Теоретическая концепция нулевой защиты каждого вызова метода имеет некоторые достоинства, если она хорошо опубликована и последовательно применяется.Тем не менее, код живет в экосистеме, а не в вакууме. Нулевое охраняемое поведение было бы неидиоматичным и удивительным в C ++, и поэтому его следует считать вредным. Таким образом, никто не пишет код C ++ таким образом, так что не делайте этого! В качестве контр-примера, однако, обратите внимание, что вызов
free()
илиdelete
для aNULL
в C и C ++ гарантированно будет неработоспособным.В вашем примере, вероятно, стоило бы поместить утверждение в конструктор,
msgSender
которое не является нулевым. Если конструктор называется метод наmsgSender
сразу же, то не такое утверждение не будет необходимости, поскольку она разбила бы прямо там в любом случае. Однако, поскольку он просто хранитсяmsgSender
для будущего использования, не будет очевидным, если посмотреть на трассировку стека того,SignalShutdown()
как получилось значениеNULL
, поэтому утверждение в конструкторе значительно упростит отладку.Более того, конструктор должен принимать
const IMsgSender&
ссылку, чего не может бытьNULL
.источник
Причина, по которой вас просят избегать нулевых разыменований, заключается в том, чтобы гарантировать надежность вашего кода. Примеры младших программистов давно являются лишь примерами. Любой может взломать код случайно и вызвать нулевую разыменование - особенно для глобалов и глобалов классов. В C и C ++ это еще более случайно благодаря возможности прямого управления памятью. Вы можете быть удивлены, но такие вещи случаются очень часто. Даже очень хорошо осведомленными, очень опытными и очень опытными разработчиками.
Вам не нужно все проверять на ноль, но вам нужно защищать от разыменований, которые имеют приличную вероятность быть нулевыми. Обычно это происходит, когда они выделяются, используются и разыменовываются в разных функциях. Вполне возможно, что одна из других функций будет изменена и нарушит вашу функцию. Также возможно, что одна из других функций может быть вызвана не по порядку (например, если у вас есть деаллокатор, который можно вызывать отдельно от деструктора).
Я предпочитаю подход, который ваши коллеги говорят вам в сочетании с использованием assert. Сбой в тестовой среде, поэтому становится более очевидным, что существует проблема, которая может быть исправлена и изящно завершена в процессе производства.
Вам также следует использовать надежный инструмент для проверки правильности кода, такой как укрытие или укрепление. И вы должны учитывать все предупреждения компилятора.
Изменить: как уже упоминали другие, молчаливый сбой, как в нескольких примерах кода, как правило, тоже неправильно. Если ваша функция не может восстановить значение, равное нулю, она должна вернуть ошибку (или сгенерировать исключение) вызывающей стороне. Вызывающая сторона несет ответственность за исправление порядка вызовов, восстановление или возврат ошибки (или выдачу исключения) вызывающей стороне и т. Д. В конце концов, либо функция способна изящно восстанавливаться и двигаться дальше, изящно восстанавливаться и давать сбой (например, сбой транзакции в базе данных из-за внутренней ошибки для одного пользователя, но не из-за выхода из нее), либо функция определяет, что состояние приложения повреждено и неустранимый и приложение выходит.
источник
NULL
проверки «за рулем» являются способом сделать это, но я ценю услышать мнение кого-то за пределами моего рабочего места, который в основном говорит: «Эй. Не кажется слишком необоснованным».Использование указателя вместо ссылки скажет мне, что
msgSender
это только вариант, и здесь проверка на ноль будет правильной. Фрагмент кода слишком медленный, чтобы решить это. Может быть, есть другие элементы,PowerManager
которые являются ценными (или тестируемыми) ...Выбирая между указателем и ссылкой, я тщательно взвешиваю оба варианта. Если мне нужно использовать указатель для члена (даже для закрытых членов), я должен принимать
if (x)
процедуру каждый раз, когда разыменую ее.источник
Это зависит от того, что вы хотите случиться.
Вы написали, что работаете над «устройством бытовой электроники». Если каким - то образом, ошибка , вводится путем установки
msgSender_
наNULL
, вы хотитеSignalShutdown
но продолжая остальную часть его работы, илиВ зависимости от воздействия не отправляемого сигнала отключения, вариант 1 может быть приемлемым вариантом. Если пользователь может продолжать слушать свою музыку, но на дисплее по-прежнему отображается заголовок предыдущего трека, это может быть предпочтительным для полного сбоя устройства.
Конечно, если вы выберете вариант 1,
assert
(как рекомендовано другими) жизненно важно, чтобы уменьшить вероятность того, что такая ошибка будет незаметно появляться во время разработки.if
Нулевой охранник только там для смягчения отказа в использовании продукции.Лично я также предпочитаю подход «ранний сбой» для производственных сборок, но я разрабатываю программное обеспечение для бизнеса, которое можно легко исправить и обновить в случае ошибки. Для устройств бытовой электроники это может быть не так просто.
источник