Друзья-разработчики, приглашаем вас продолжить захватывающее путешествие по дебрям кода Intel OpenVINO! Вооружившись статическим анализатором аки детективы, мы разоблачим наиболее коварные, интересные ошибки и опечатки, а также их скрытые тайны, оставшиеся за рамками первой части статьи.
Итак, в первой части мы с вами рассмотрели множество поучительных опечаток в коде проекта OpenVINO. Поучительных, потому как опечатки всегда будут иметь место в жизни разработчика и от них никуда не денешься, однако проблема решаема. С помощью технологии статического анализа, который нам подсказывает о наличии подобных ошибок, мы можем составить определённую статистику о том, где программист чаще совершает такие ошибки. Когда выявлены соответствующие паттерны, борьба с опечатками становится в разы эффективнее. А если вам интересно, где чаще всего встречаются опечатки — у нас есть интересная статья, которую я рекомендую вам прочитать.
Однако, представьте, если бы мы умели по щелчку пальцев включать всю свою внимательность и выключать все отвлекающие факторы. Тогда наш код был бы идеален, и опечатки бы исчезли как вид. Но давайте помечтаем дальше... Если бы мы ещё могли предвидеть все возможные сценарии использования кода, учитывать все взаимосвязи между компонентами и предугадывать каждую ошибку... Написанные нами программы работали бы идеально. Однако я знаю только один "язык программирования", где всё это возможно, и это — HTML.
Но мы всё же имеем дело с С++. Здесь даже самый опытный программист может ошибиться, хм... да, собственно, где и как угодно. И вот, разделив все предупреждения на "до" и "после" и разобравшись с опечатками, давайте посмотрим и на другие, не менее интересные и разнообразные ошибки.
Написание и публикация этой статьи не имеют цели обесценить труд программистов, занимающихся разработкой данного продукта. Цель: популяризация статических анализаторов кода, которые полезны даже для качественных и состоявшихся проектов. Даже больше, для Open Source проектов (и не только) мы предоставляем бесплатные лицензии. Подробнее можно узнать здесь.
Вообще, какого-либо огромного количества ошибок во время анализа найдено не было. Всё интересное и важное уместилось всего в паре статей. Код весьма красивый и защищённый. Всё так же, коммит, на котором я собирал проект для проверки: 2d8ac08.
Ну, с формальностями закончили, можно вернуться к разбору ошибок. Как и ожидалось, сложность багов повышается, а вместе со сложностью возрастает запутанность кода, в котором нам придётся разобраться. Местами код вообще довольно странный — непонятно, как или почему так написан — но предлагаю начать с более-менее простого, а затем... Ну, вы уже и сами всё скоро увидите.
Фрагмент N1
template <typename T>
void ROIAlignForward(....)
{
int64_t roi_cols = 4; // <=
int64_t n_rois = nthreads / channels / pooled_width / pooled_height;
// (n, c, ph, pw) is an element in the pooled output
for (int64_t n = 0; n < n_rois; ++n)
{
....
if (roi_cols == 5) // <=
{
roi_batch_ind = static_cast<int64_t>(offset_bottom_rois[0]);
offset_bottom_rois++;
}
....
}
....
}Предупреждение анализатора: V547 Expression 'roi_cols == 5' is always false. experimental_detectron_roi_feature_extractor.cpp 211
Проверка if (roi_cols == 5) действительно всегда будет возвращать false, а код в теле никогда не будет достижим. Связано это с тем, что значение переменной roi_cols никак не изменяется между её декларацией и проверкой в условии.
Фрагмент N2
bool is_valid_model(std::istream& model)
{
// the model usually starts with a 0x08 byte
// indicating the ir_version value
// so this checker expects at least 3 valid ONNX keys
// to be found in the validated model
const size_t EXPECTED_FIELDS_FOUND = 3u;
std::unordered_set<::onnx::Field, std::hash<int>> onnx_fields_found = {};
try
{
while (!model.eof() && onnx_fields_found.size() < // <=
EXPECTED_FIELDS_FOUND )
{
const auto field = ::onnx::decode_next_field(model);
if (onnx_fields_found.count(field.first) > 0)
{
// if the same field is found twice, this is not a valid ONNX model
return false;
}
else
{
onnx_fields_found.insert(field.first);
::onnx::skip_payload(model, field.second);
}
}
return onnx_fields_found.size() == EXPECTED_FIELDS_FOUND;
}
catch (...)
{
return false;
}
}Предупреждение анализатора: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. onnx_model_validator.cpp 168
Обнаружена довольно интересная и редкая ошибка, из-за которой цикл может быть бесконечным.
Работая с объектами класса std::istream, проверки !model.eof() для выхода из цикла while может быть недостаточно. В случае возникновения сбоя при чтении данных, все последующие вызовы функции eof будут возвращать исключительно false, что может привести к зацикливанию.
Для того, чтобы не допустить этой проблемы, можно было бы использовать перегруженный оператор bool в условии цикла следующим образом:
while (model && onnx_fields_found.size() < EXPECTED_FIELDS_FOUND)
{
....
}Фрагмент N3
NamedOutputs pad3d(const NodeContext& node)
{
....
// padding of type int feature only supported by paddle 'develop'
// version(>=2.1.0)
if (node.has_attribute("paddings")) // <=
{
auto paddings_vector = node.get_attribute<
std::vector<int32_t>
>("paddings");
PADDLE_OP_CHECK(node, paddings_vector.size() == 6,
"paddings Params size should be 6 in pad3d!");
paddings = paddings_vector;
}
else if (node.has_attribute("paddings")) // <=
{
auto padding_int = node.get_attribute<int32_t>("paddings");
for (int i = 0; i < 6; i++)
paddings[i] = padding_int;
}
else
{
PADDLE_OP_CHECK(node, false, "Unsupported paddings attribute!");
}
....
}Предупреждение анализатора: V517 [CERT-MSC01-C] The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 22, 26. pad3d.cpp 22
Первое и второе условия конструкций if совпадают, поэтому код в then-ветке второго if всегда недостижим. При этом можно заметить, что ветки выполняют разную логику: первая пытается прочитать атрибут paddings в виде вектора чисел типа int32_t (paddings_vector), вторая – прочитать этот же атрибут в виде числа типа int32_t (padding_int).
Мне тяжело определить, как именно должен выглядеть корректный код в этой ситуации. Однако давайте попробуем предположить. Код располагается в модуле OpenVINO Paddle Frontend, который парсит модель, сформированную фреймворком PaddlePaddle. Если поискать имя 'pad3d' в этом проекте, то можно найти следующее описание:
Parameters:
padding (Tensor|list[int]|tuple[int]|int): The padding size with data type
``'int'``. If is ``'int'``, use the same padding in all dimensions.
Else [len(padding)/2] dimensions of input will be padded.
The pad has the form (pad_left, pad_right, pad_top,
pad_bottom, pad_front, pad_back).Из этого предположим, что padding — это вариант, и нужно перебрать две интересующие альтернативы — std::vector<int32_t> и int32_t. Это можно попробовать сделать следующим способом:
auto paddings = std::vector<int32_t>(6, 0);
if (node.has_attribute("paddings"))
{
auto paddings_attr = node.get_attribute_as_any("paddings");
if (paddings_attr.is<std::vector<int32_t>>())
{
auto paddings_vector = paddings_attr.as<std::vector<int32_t>>();
PADDLE_OP_CHECK(node, paddings_vector.size() == 6,
"paddings Params size should be 6 in pad3d!");
paddings = std::move(paddings_vector);
}
else if (paddings_attr.is<int32_t>())
{
auto padding_int = paddings_attr.as<int32_t>();
if (padding_int != 0)
{
std::fill(paddings.begin(), paddings.end(), padding_int);
}
}
}Также изучение исходников PaddlePaddle навеяло мысль о том, что в написании атрибута закралась опечатка, и он должен называться padding, а не paddings. Но это не точно :) В любом случае рекомендую разработчиками обратить внимание на этот код.
Фрагмент N4
template <typename T>
std::basic_string<T> get_model_extension() {}Предупреждение анализатора: V591 [CERT-MSC52-CPP] Non-void function should return a value. graph_iterator_flatbuffer.hpp 29
Как можно заметить, функция имеет пустое тело и ничего не возвращает, хотя возвращаемый тип указан как std::basic_string<T>. Имеем неопределённое поведение в программе.
Данный фрагмент кода как будто бы появился из сериала "Секретные материалы". Но на самом деле всё достаточно просто. Если посмотреть буквально пару строк ниже, то можно заметить специализации этого шаблона функции:
template <>
std::basic_string<char> get_model_extension<char>();
#if defined(OPENVINO_ENABLE_UNICODE_PATH_SUPPORT) \
&& defined(_WIN32)
template <>
std::basic_string<wchar_t> get_model_extension<wchar_t>();
#endifИ у этих специализаций есть вполне конкретные тела, которые возвращают вполне конкретные объекты:
template <>
std::basic_string<char>
ov::frontend::tensorflow_lite::get_model_extension<char>()
{
return ::tflite::ModelExtension();
}
#if defined(OPENVINO_ENABLE_UNICODE_PATH_SUPPORT) \
&& defined(_WIN32)
template <>
std::basic_string<wchar_t> ov::frontend::
tensorflow_lite::get_model_extension<wchar_t>()
{
return util::string_to_wstring(::tflite::ModelExtension());
}
#endifЗначит, программа будет вполне корректно работать со специализациями от char и wchar_t и творить что угодно при других. А их может быть еще три: char8_t, char16_t, char32_t.
Да, в коде может не быть использования таких специализаций шаблона, но мы же с вами за надёжный и безопасный код. И хотелось бы, чтобы компилятор остановил нас ещё на этапе компиляции, если кто-то напишет такой код. Сделать это очень просто, нужно всего лишь превратить определение шаблона функции в декларацию:
template <typename T>
std::basic_string<T> get_model_extension();Теперь при попытке вызова специализации от char8_t, char16_t или char32_t компилятор выдаст ошибку, потому что без тела он не сможет сделать нужное инстанцирование.
Ещё несколько подобных предупреждений:
Фрагмент N5
template <typename TReg>
int getFree(int requestedIdx)
{
if (std::is_base_of<Xbyak::Mmx, TReg>::value)
{
auto idx = simdSet.getUnused(requestedIdx);
simdSet.setAsUsed(idx);
return idx;
}
else if ( std::is_same<TReg, Xbyak::Reg8>::value
|| std::is_same<TReg, Xbyak::Reg16>::value
|| std::is_same<TReg, Xbyak::Reg32>::value
|| std::is_same<TReg, Xbyak::Reg64>::value)
{
auto idx = generalSet.getUnused(requestedIdx);
generalSet.setAsUsed(idx);
return idx;
}
else if (std::is_same<TReg, Xbyak::Opmask>::value)
{
return getFreeOpmask(requestedIdx);
}
}Предупреждение анализатора: V591 [CERT-MSC52-CPP] Non-void function should return a value. registers_pool.hpp 229
Ещё один фрагмент с функцией, которая не возвращает значение на всех путях исполнения. Если вы считаете, что c вашей программой не произойдёт ничего страшного, если вы забыли return в функции, то вы ошибаетесь. Даже если возвращаемый тип встроенный, код содержит неопределённое поведение. Советую также к прочтению эту статью о том, какую злую шутку может сыграть с вами компилятор в этой ситуации.
Вернёмся к нашему примеру. Начиная с C++17, этот код можно сделать лучше. Во-первых, как можно заметить, условия представляют из себя выражения, вычисляемые на этапе компиляции. Поэтому следует воспользоваться конструкцией if constexpr. Она позволит не компилировать код в тех ветках, в которых условие вычисляется как false. Во-вторых, с помощью static_assert можно защитить код от тех специализаций, для которых в текущем коде не возвращается значение:
template <typename TReg>
int getFree(int requestedIdx)
{
if constexpr (std::is_base_of<Xbyak::Mmx, TReg>::value) { .... }
....
else
{
// until C++23
static_assert(sizeof(TReg *) == 0, "Your message.");
// since C++23
static_assert(false, "Your message.");
}
}В исправленном примере приведены два способа написать static_assert в зависимости от используемой версии стандарта. Дело в том, что до C++23 вариант static_assert(false, "...") в шаблоне функции всегда приводил к ошибке этапа компиляции.
До C++17 можно поправить код, добавив перегрузки шаблона функции getFree и воспользовавшись std::enable_if:
template <typename TReg,
std::enable_if_t< std::is_base_of<Xbyak::Mmx, TReg>::value,
int > = 0>
int getFree(int requestedIdx)
{
auto idx = simdSet.getUnused(requestedIdx);
simdSet.setAsUsed(idx);
return idx;
}
template <typename TReg,
std::enable_if_t< std::is_same<TReg, Xbyak::Reg8>::value
|| std::is_same<TReg, Xbyak::Reg16>::value
|| std::is_same<TReg, Xbyak::Reg32>::value
|| std::is_same<TReg, Xbyak::Reg64>::value,
int > = 0>
int getFree(int requestedIdx)
{
auto idx = generalSet.getUnused(requestedIdx);
generalSet.setAsUsed(idx);
return idx;
}
template <typename TReg,
std::enable_if_t< std::is_same<TReg, Xbyak::Opmask>::value,
int > = 0>
int getFree(int requestedIdx)
{
return getFreeOpmask(requestedIdx);
}Фрагмент N6
template <>
void RandomUniform<x64::avx512_core>::initVectors()
{
....
if (m_jcp.out_data_type.size() <= 4)
{
static const uint64_t n_inc_arr[8] = { 0, 1, 2, 3, 4, 5, 6, 7 };
mov(r64_aux, reinterpret_cast<uintptr_t>(n_inc_arr));
}
else
{
static const uint64_t n_inc_arr[8] =
{ 0, 1, 2, 3, 4, 5, 6, 7 }; // TODO: i64
mov(r64_aux, reinterpret_cast<uintptr_t>(n_inc_arr));
}
....
}Предупреждение анализатора: V523 The 'then' statement is equivalent to the 'else' statement. random_uniform.cpp 120
Здесь всё просто, но непонятно. Что в ветке if, что в else, в теле содержится один и тот же код. Возможно, это ошибка, связанная с копипастом, и разработчикам стоит обратить на этот фрагмент кода внимание.
Фрагмент N7
inline ParseResult parse_xml(const char* file_path)
{
....
try
{
auto xml = std::unique_ptr<pugi::
xml_document>{new pugi::xml_document{}};
const auto error_msg = [&]() -> std::string {....}();
....
return {std::move(xml), error_msg};
}
catch (std::exception& e)
{
return {std::move(nullptr), // <=
std::string("Error loading XML file: ") + e.what()};
}
}Предупреждение анализатора: V575 [CERT-EXP37-C] The null pointer is passed into 'move' function. Inspect the first argument. xml_parse_utils.hpp 249
Честно говоря, мы впервые в жизни увидели такой интересный код: в функцию std::move передали nullptr. Вообще в std::move не возбраняется передавать nullptr — он просто преобразуется в std::nullptr_t &&. Однако непонятно, зачем так делать.
Для лучшего понимания происходящего давайте заглянем в ParseResult:
struct ParseResult
{
ParseResult(std::unique_ptr<pugi::xml_document>&& xml, std::string error_msg)
: xml(std::move(xml)),
error_msg(std::move(error_msg)) {}
std::unique_ptr<pugi::xml_document> xml;
std::string error_msg{};
};Попробуем поиграть в детектива и разгадаем загадку, откуда мог появиться такой код. Скорее всего, программист сначала написал return в try-блоке: в нём конструируется объект ParseResult, перемещая умный указатель xml и копируя error_msg. Затем обработал ситуацию, когда будет выброшено исключение. В этом случае также надо вернуть какой-то объект типа ParseResult. Чтобы упростить себе жизнь, он скопировал предыдущий return, слегка модифицировав аргументы конструктора. При этом, увидев перемещение умного указателя xml, он принял решение, что и здесь надо что-то переместить. Например, nullptr.
Однако нужды в этом никакой нет. Согласно правилам C++, при выборе перегрузки конструктора компилятор будет обязан произвести некоторые неявные преобразования переданных аргументов. Например, r-value ссылка на std::unique_ptr<pugi::xml_document> не может быть привязана к r-value ссылке на std::nullptr_t. Поэтому компилятор создаст временный объект типа std::unique_ptr<pugi::xml_document>, позвав соответствующий конвертирующий конструктор. И лишь затем на этот временный объект будет привязана ссылка (параметр конструктора).
Если мы опустим std::move и передадим nullptr в конструктор, код также будет компилируемым и станет более читаемым:
return { nullptr, std::string("Error loading XML file: ") +
e.what() };И вот мы плавно переходим к работе с указателями. А анализатор поможет разобраться в ситуациях, в которых что-то пошло не так, да и в целом, как не стоит писать код.
Фрагмент N8
void GraphOptimizer::FuseFCAndWeightsDecompression(Graph &graph)
{
....
// Fusion processing
auto *multiplyInputNode = dynamic_cast<node::
Input *>(multiplyConstNode.get());
if (!multiplyInputNode)
{
OPENVINO_THROW("Cannot cast ", multiplyInputNode->getName(), // <=
" to Input node.");
}
fcNode->fuseDecompressionMultiply(multiplyInputNode->getMemoryPtr());
if (withSubtract)
{
auto *subtractInputNode = dynamic_cast<node::
Input *>(subtractConstNode.get());
if (!subtractInputNode)
{
OPENVINO_THROW("Cannot cast ", subtractInputNode->getName(), // <=
" to Input node.");
}
fcNode->fuseDecompressionSubtract(subtractInputNode->getMemoryPtr());
}
if (withPowerStatic)
{
auto *eltwiseNode = dynamic_cast<node::
Eltwise *>(powerStaticNode.get());
if (!eltwiseNode)
{
OPENVINO_THROW("Cannot cast ", eltwiseNode->getName(), // <=
" to Eltwise node.");
}
}
....
}Предупреждения анализатора:
Часто вижу подобные ошибки в различных проектах, поэтому и решил обратить на них внимание. Суть в том, что программисты редко тестируют обработчики ошибок :)
Представим на секунду, что результат dynamic_cast вернёт нулевой указатель. Тогда при формировании исключения будет использован тот самый нулевой указатель в попытке вызвать функцию getName. Получаем неопределённое поведение, из-за которого обработка исключительной ситуации может превратиться в фатальную для программы ошибку.
Фрагмент N9
void Defer(Task task)
{
auto &stream = *(_streams.local()); // <=
stream._taskQueue.push(std::move(task));
if (!stream._execute)
{
stream._execute = true;
try
{
while (!stream._taskQueue.empty())
{
Execute(stream._taskQueue.front(), stream);
stream._taskQueue.pop();
}
}
catch (...)
{
}
stream._execute = false;
}
}Предупреждение анализатора: V758 The 'stream' reference becomes invalid when smart pointer returned by a function is destroyed. cpu_streams_executor.cpp 410
На самом деле, в этом примере всё хорошо, и ссылка не будет "висячей". Происходит это потому, что shared_ptr, который возвращает функция local, предварительно сохраняется в контейнер _stream_map, время жизни которого больше, чем у ссылки:
class CustomThreadLocal : public ThreadLocal<std::shared_ptr<Stream>>
{
....
public:
std::shared_ptr<Stream> local()
{
....
if (stream == nullptr)
{
stream = std::make_shared<Impl::Stream>(_impl);
}
auto tracker_ptr = std::make_shared<
CustomThreadLocal::ThreadTracker
>(id);
t_stream_count_map[(void*)this] = tracker_ptr;
auto new_tracker_ptr = tracker_ptr->fetch();
_stream_map[new_tracker_ptr] = stream; // <=
return stream;
}
private:
....
std::map<std::shared_ptr<CustomThreadLocal::ThreadTracker>,
std::shared_ptr<Impl::Stream>> _stream_map;
....
};Однако выступлю в защиту анализатора: лучше всё же сохранить результат функции local в переменную функции Defer. Давайте представим, что код немного поменялся. Начнёт происходить следующее:
Сохранив результат в переменную, мы продлим время жизни объекта до выхода из области видимости и избавимся от потенциальной проблемы:
void Defer(Task task)
{
auto local = _streams.local();
auto &stream = *local;
....
}Фрагмент N10
~DIR()
{
if (!next)
delete next;
next = nullptr;
FindClose(hFind);
}Предупреждение анализатора: V575 [CERT-EXP37-C] The null pointer is passed into 'operator delete'. Inspect the argument. w_dirent.h 94
С первого взгляда на этот код может быть непонятно, а в чём именно заключается ошибка. А дело в том, что память по указателю next не освободится, так как проверка была написана неверно. Анализатор косвенно сообщает об этом, увидев передачу нулевого указателя в оператор delete. Более того, проверять указатель вообще не имеет смысла, т.к. оператор delete корректно обрабатывает нулевые указатели.
Перед вами пример кода, который практически полностью совпадает с примером, приведённым моим коллегой в статье: "Ошибка настолько проста, что программисты её не замечают". Рекомендую вам ознакомиться с ней. Если вы читали её ранее и думали, что приведённый там код синтетический, и в реальной жизни такое встречаться не может — то вот вам прямое доказательство :)
Исправленный код:
~DIR()
{
delete next;
next = nullptr;
FindClose(hFind);
}Фрагмент N11
void ov_available_devices_free(ov_available_devices_t* devices)
{
if (!devices)
{
return;
}
for (size_t i = 0; i < devices->size; i++)
{
if (devices->devices[i])
{
delete[] devices->devices[i];
}
}
if (devices->devices)
delete[] devices->devices;
devices->devices = nullptr;
devices->size = 0;
}Предупреждение анализатора: V595 The 'devices->devices' pointer was utilized before it was verified against nullptr. Check lines: 271, 274. ov_core.cpp 271
Для начала разберёмся, что из себя представляет тип ov_available_devices_t:
typedef struct {
char** devices; //!< devices' name
size_t size; //!< devices' number
} ov_available_devices_t;Как можно понять из названия, функция освобождает переданный в неё объект типа ov_available_devices_t. Только разработчик допустил при этом одну ошибку.
Сначала в цикле происходит освобождение каждого указателя в массиве devices->devices. При этом подразумевается, что указатель на массив всегда ненулевой. Затем разработчик усомнился в этом и после цикла решил проверить его перед передачей в оператор delete[]. Только в цикле он забыл о таком предположении, в результате имеем разыменование потенциально нулевого указателя.
Поправленный код:
void ov_available_devices_free(ov_available_devices_t* devices)
{
if (!devices || !devices->devices)
{
return;
}
for (size_t i = 0; i < devices->size; i++)
{
delete[] devices->devices[i];
}
delete[] devices->devices;
devices->devices = nullptr;
devices->size = 0;
}Как вы можете заметить, я убрал все проверки указателей перед передачей в оператор delete[], т.к. последний умеет с ними работать.
Анализатор также обнаружил еще несколько похожих мест:
Фрагмент N12
char* str_to_char_array(const std::string& str)
{
std::unique_ptr<char> _char_array(new char[str.length() + 1]); // <=
char* char_array = _char_array.release(); // <=
std::copy_n(str.c_str(), str.length() + 1, char_array);
return char_array;
}Как и было обещано в начале, — как вишенка на торте, — тот самый код, при прочтении которого у ревьюера может возникнуть вопрос: "Что ты такое?".
Перед нами функция-конвертер, копирующая строку std::string в динамически аллоцированный буфер char*. Буфер создается при помощи оператора new[], владение которым передаётся умному указателю. В целом, очень верная стратегия – обернуть сырой указатель, ведь если что-то пойдёт не так, то умный указатель обо всём позаботится.
Однако обратите внимание, какую именно специализацию умного указателя используют — std::unique_ptr<char>. Её деструктор освобождает переданный ресурс с помощью оператора delete. Собственно, об этом нас и предупреждает анализатор:
V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. ov_core.cpp 14
Правильно использовать в таких ситуациях специализацию std::unique_ptr<char[]>:
char* str_to_char_array(const std::string& str)
{
std::unique_ptr<char[]> _char_array(new char[str.length() + 1]);
....
}P.S. Читатель может возразить, что в этой ситуации ничего страшного не произойдёт, т.к. владение ресурсом отдаётся возвращаемому объекту прямо следующей строкой. Действительно, это так. Но всё же это "код с запахом", и его проблема в том, что у других разработчиков может возникнуть желание использовать такое объявление умного указателя в других местах.
И, к сожалению, процесс размножения уже был запущен:
И если между захватом ресурса и его передачей во владение сырому указателю произойдёт бросающая исключение операция, то получим неопределённое поведение.
P.P.S. Если в проекте используется стандарт C++14 и выше, можно заменить код на следующий:
char* str_to_char_array(const std::string& str)
{
auto _char_array = std::make_unique<char[]>(str.length() + 1);
char* char_array = _char_array.release();
....
}Во-первых, мы избавились от явного использования оператора new[] в коде, переложив всё на std::make_unique. Во-вторых, auto выведет за нас корректную специализацию умного указателя на основе инициализатора.
Однако такой код, помимо динамической аллокации, также заполнит массив нулями. Раз уж далее происходит полная перезапись массива, то можно сэкономить ресурсы и не выполнять его инициализацию. Начиная с C++20, для этого доступна функция std::make_unique_for_overwrite:
char* str_to_char_array(const std::string& str)
{
auto _char_array = std::make_unique_for_overwrite<char[]>(
str.length() + 1
);
char* char_array = _char_array.release();
....
}Некоторые примеры из проекта OpenVINO меня сильно удивили. Думаю, и вас тоже. Однако, как я уже писал ранее, все важные ошибки, на которые стоило бы обратить внимание разработчикам данного проекта, уместились в паре статьей. Чаще всего это были опечатки, что является распространённой проблемой среди программистов и встречаются почти в любом проекте. OpenVINO — проект внушительный и столько малое (на мой взгляд) количество серьёзных ошибок говорит только о том, что код довольно качественно написан.
Надеюсь, вам так же, как и мне, было интересно разглядывать все эти примеры кода, а разбор некоторых ошибок оказался полезным опытом.
Всю информацию мы, конечно же, передали разработчикам и будем надеяться, что баги в ближайшем будущем исправят.
И, как у нас уже исторически сложилось, предлагаю попробовать наш анализатор PVS-Studio. Для Open Source проектов у нас предоставляется бесплатная лицензия.
Берегите себя и всего доброго!