Так получилось, что в один период времени я обсуждал в интернете, казалось бы, разные темы: бесплатные альтернативы Matlab для университетов и студентов, и поиск ошибок в алгоритмах с помощью статического анализа кода. Все эти обсуждения объединило ужасное качество кода современных программ. В частности, качество софта для математиков и учёных. Тут же возникает вопрос о доверии к расчётам и исследованиям, проведённым с помощью таких программ. Попробуем поразмыслить на эту тему и поискать ошибки.
Введение
Начать я хочу с определения термина «алгоритм». Алгоритм — это набор инструкций, описывающих порядок действий исполнителя для достижения некоторого результата (Wikipedia). Таким образом, не стоит делить исходный код на алгоритмы и весь остальной код. Например, алгоритмы сортировки — это точно такой же код, как и открытие файла, поиск символа в строке и т.п. Код может содержать ошибку и, к счастью, много ошибок можно выявить на начальном этапе, воспользовавшись инструментами статического анализа кода.
Тем не менее, для поиска, так называемых, «алгоритмических» ошибок я решил проанализировать код нескольких математических пакетов. В таком коде много функций, в которых просто запрограммированы какие-нибудь математический формулы. Оказывается, есть люди, которые такое даже за код не считают. И, соответственно, какие там могут быть ошибки.
Для выявления всех дефектов кода, представленных в статье, использовался статический анализатор PVS-Studio версии 6.15, работающий в Windows/Linux с языками программирования C/C++/C#.
Ошибки из 3rd party
Эта история началась с поиска ошибок в проекте Point Cloud Library (PCL, GitHub). Не ставив себе цели найти много ошибок и написать статью, я просто просматривал отчёт и нашёл очень интересную ошибку:
V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. sparsematrix.inl 212
template<class T>
SparseMatrix<T>& SparseMatrix<T>::operator *= (const T& V)
{
for( int i=0 ; i<rows ; i++ )
for( int ii=0 ; ii<rowSizes[i] ; i++ )
m_ppElements[i][ii].Value *= V;
return *this;
}В перегруженном операторе "*=" реализовали умножение всех элементов матрицы на некоторое значение V. Автор кода допустил очень серьёзную ошибку для данного алгоритма, из-за которой изменяется только первый столбец матрицы, а также возможен бесконечный цикл с выходом за границы массива.
Этот код оказался из математической библиотеки Poisson Surface Reconstruction. Я убедился, что ошибка до сих по присутствует в последней версии кода. Страшно подумать, сколько ещё проектов включают в себя эту библиотеку.
Вот ещё один странный фрагмент кода:
V607 Ownerless expression 'j < remains'. allocator.h 120
void rollBack(const AllocatorState& state){
....
if(state.index<index){
....
for(int j=0;j<remains;j++){
memory[index][j].~T();
new(&memory[index][j]) T();
}
index=state.index;
remains=state.remains;
}
else{
for(int j=0;j<state.remains;j<remains){ // <=
memory[index][j].~T();
new(&memory[index][j]) T();
}
remains=state.remains;
}
....
}Подозреваю, что этот странный цикл не часто выполняется, раз до сих пор остаётся в коде. Но у кого-то наверняка случались непонятные зависания с аварийным завершением программы. Некое представление о качестве кода сформировалось. Теперь перейдём к проекту покрупнее — Scilab, где нас ждёт настоящая головная боль.
Scilab
О проекте
Scilab — пакет прикладных математических программ, предоставляющий открытое окружение для инженерных (технических) и научных расчётов. Эта среда разработки является одной из общедоступных альтернатив Matlab, которая широко используется в разных учреждениях и научных исследованиях. Ещё одной популярной альтернативой Matlab является GNU Octave, и ранее мы уже обращали внимание на эти проекты:
- Проверка Scilab (март 2014);
- Проверка GNU Octave (август 2015).
Перед написанием новой статьи о Scilab я прочитал старую и сделал всего два вывода:
- За 3 года не исправили всего парочку мест («Зачем исправлять неопределённое поведение, если и так всё работает?» — видимо, подумали разработчики);
- В проекте появилось много новых ошибок. Я решил поместить в статью только пару десятков, чтобы не слишком утомить читателя.
В исходниках Scilab сразу присутствует проектный файл для Visual Studio, поэтому его можно было открыть и проверить проект в один клик, что я и сделал.
Красивые опечатки
V530 The return value of function 'back' is required to be utilized. sci_mscanf.cpp 274
types::Function::ReturnValue sci_mscanf(....)
{
....
std::vector<types::InternalType*> pITTemp = std::vector<...>();
....
case types::InternalType::ScilabString :
{
....
pITTemp.pop_back(); // <=
pITTemp.push_back(pType);
}
break;
case types::InternalType::ScilabDouble :
{
....
pITTemp.back(); // <= ???
pITTemp.push_back(pType);
}
break;
....
}Похоже, автодополнение кода сыграло с программистом злую шутку. В коде функции sci_mscanf всегда удаляют последний элемент вектора перед добавлением нового, но в одном месте программист допустил ошибку, позвав функцию back(), вместо pop_back(). Вызывать функцию back() таким образом нет никакого смысла.
V595 The 'Block.inptr' pointer was utilized before it was verified against nullptr. Check lines: 478, 479. sci_model2blk.cpp 478
types::Function::ReturnValue sci_model2blk(....)
{
....
Block.inptr[i] = MALLOC(size);
if (Block.inptr == nullptr)
{
freeBlock(&Block);
Scierror(888, _("%s : Allocation error.\n"), name.data());
return types::Function::Error;
}
memset(Block.inptr[i], 0x00, size);
....
}Очень интересный случай опечатки, из-за которой контроль за выделением памяти перестал работать. Скорее всего, правильный код должен быть таким:
Block.inptr[i] = MALLOC(size);
if (Block.inptr[i] == nullptr)
{
....
}V595 The 'pwstLines' pointer was utilized before it was verified against nullptr. Check lines: 78, 79. mgetl.cpp 78
int mgetl(int iFileID, int iLineCount, wchar_t ***pwstLines)
{
*pwstLines = NULL;
....
*pwstLines = (wchar_t**)MALLOC(iLineCount * sizeof(wchar_t*));
if (pwstLines == NULL)
{
return -1;
}
....
}На удивление похожая ошибка. Автор кода не досчитался звёздочек, поэтому в условии проверяется не тот указатель.
V595 The 'array_size' pointer was utilized before it was verified against nullptr. Check lines: 67, 68. diary_manager.cpp 67
wchar_t **getDiaryFilenames(int *array_size)
{
*array_size = 0;
if (SCIDIARY)
{
std::list<std::wstring> wstringFilenames = SCIDIARY->get....
*array_size = (int)wstringFilenames.size();
if (array_size > 0)
{
....
}
....
}Стабильность — признак мастерства. Программист снова забыл разыменовать указатель, из-за чего с нулём сравнивается не размер некоторого массива, а указатель на эту переменную.
V501 There are identical sub-expressions 'strncmp(tx, "%pi", 3) == 0' to the left and to the right of the '||' operator. stringtocomplex.c 276
static int ParseNumber(const char* tx)
{
....
else if (strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0
|| strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0
|| strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0
|| strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0
|| strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0
))
{
return 4;
}
else if (strlen(tx) >= 3
&& (strncmp(tx, "+%e", 3) == 0
|| strncmp(tx, "-%e", 3) == 0
|| strncmp(tx, "%pi", 3) == 0 // <=
|| strncmp(tx, "Nan", 3) == 0
|| strncmp(tx, "Inf", 3) == 0
|| strncmp(tx, "%pi", 3) == 0)) // <=
{
return 3;
}
....
}Эта функция содержит некий код для парсинга чисел. Анализатор нашёл подозрительное сравнение с двумя одинаковыми строками "%pi". Глядя на соседний фрагмент кода, можно предположить, что вместо продублированной строки могла быть строка "-%pi" или "-Inf". Также не исключено, что это просто лишняя скопированная строчка кода, но тогда её лучше удалить.
Приоритеты операций
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. sci_sparse.cpp 49
types::Function::ReturnValue sci_sparse(....)
{
bool isValid = true;
....
for (int i = 0 ; isValid && i < in.size() ; i++)
{
switch (in[i]->getType())
{
case types::InternalType::ScilabBool :
case types::InternalType::ScilabSparseBool :
{
isValid = (i == (in.size() > 1) ? 1 : 0);
}
....
}Ошибки с приоритетами операций очень распространены в современном коде. (см. статью "Логические выражения в C/C++. Как ошибаются профессионалы").
В приведённом фрагменте кода тоже есть ошибка, но из-за очень большого везения код с ошибкой работает, как ожидал программист. Только из-за того, что в сравнении участвуют элементы массива с индексами 0 и 1, а целочисленными представлениями истины и лжи тоже являются значения 0 и 1, данный фрагмент кода чудесным образом всё равно работает правильно.
Следует переписать код на правильный приоритет операций:
isValid = (i == (in.size() > 1 ? 1 : 0));V590 Consider inspecting the 'iType != — 1 && iType == 8' expression. The expression is excessive or contains a misprint. scilabview.cpp 175
void ScilabView::createObject(int iUID)
{
int iType = -1;
int *piType = &iType;
getGraphicObjectProperty(....);
if (iType != -1 && iType == __GO_FIGURE__)
{
m_figureList[iUID] = -1;
setCurrentFigure(iUID);
}
....
}В этом фрагменте присутствует ошибка с приоритетом операций, которая также рассмотрена в предложенной ранее статье.
Условное подвыражение (iType != -1) никак не влияет на результат всего условного выражения. Убедиться в ошибке можно с помощью построения таблицы истинности для этого примера.
Ещё один такой пример:
- V590 Consider inspecting the 'iObjectType != — 1 && iObjectType == 5' expression. The expression is excessive or contains a misprint. sci_unglue.c 90
Неправильные сообщения об ошибках
В предыдущей статье об ошибках в Scilab тоже был не маленький раздел про ошибки при печати сообщений. На свежей кодовой оказалось тоже достаточно много ошибок такого типа.
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 159, 163. cdfbase.c 159
void cdf_error(char const* const fname, int status, double bound)
{
switch (status)
{
....
case 10:
if (strcmp(fname, "cdfchi") == 0) // <=
{
Scierror(999
_("%s: cumgam returned an error\n"), fname);
}
else if (strcmp(fname, "cdfchi") == 0) // <=
{
Scierror(999,
_("%s: gamma or inverse gamma routine failed\n"), fname);
}
break;
....
}В Scilab есть большой набор cdf функций. В представленном фрагменте кода выполняется интерпретация кодов возврата этих функций. И вот беда — некое предупреждение об ошибке никогда не выводится из-за опечатки в имени функции. Поиск по этому сообщению приводит к функции cdfgam. Хочется посочувствовать пользователям, которые работали с этой функцией и не могли узнать о некоторых проблемах из-за опечатки авторов математического пакета.
V510 The 'Scierror' function is not expected to receive class-type variable as third actual argument. sci_winqueryreg.cpp 149
const std::string fname = "winqueryreg";
types::Function::ReturnValue sci_winqueryreg(....)
{
....
if (rhs != 2 && rhs != 3)
{
Scierror(77, _("%s: Wrong number...\n"), fname.data(), 2, 3);
return types::Function::Error;
}
....
else
{
Scierror(999, _("%s: Cannot open Windows regist..."), fname);
return types::Function::Error;
}
....
}При печати строки в одном месте забыли вызвать метод data().
V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 48
int sci_scinotes(char * fname, void* pvApiCtx)
{
....
try
{
callSciNotesW(NULL, 0);
}
catch (GiwsException::JniCallMethodException exception)
{
Scierror(999, "%s: %s\n", fname,
exception.getJavaDescription().c_str());
}
catch (GiwsException::JniException exception)
{
Scierror(999, "%s: %s\n", fname,
exception.whatStr().c_str());
}
....
}Исключение перехвачено по значению. Это значит, что с помощью конструктора копирования будет сконструирован новый объект и часть информации об исключении будет потеряна. Правильным вариантом является перехват исключений по ссылке.
Таких мест найдено несколько:
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_builddoc.cpp 270
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_closescinotesfromscilab.cpp 45
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_closescinotesfromscilab.cpp 50
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 52
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 263
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 272
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 349
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 353
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 365
- V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 369
- V746 Type slicing. An exception should be caught by reference rather than by value. visitor_common.cpp 1743
- V746 Type slicing. An exception should be caught by reference rather than by value. overload.cpp 135
Странный код
Странный код, потому что непонятно, зачем так писать и как это лучше исправить.
V523 The 'then' statement is equivalent to the 'else' statement. data3d.cpp 51
void Data3D::getDataProperty(int property, void **_pvData)
{
if (property == UNKNOWN_DATA_PROPERTY)
{
*_pvData = NULL;
}
else
{
*_pvData = NULL;
}
}Вот такая нехитрая функция, которая всегда обнуляет указатель.
V575 The 'memset' function processes '0' elements. Inspect the third argument. win_mem_alloc.c 91
void *MyHeapAlloc(size_t dwSize, char *file, int line)
{
LPVOID NewPointer = NULL;
if (dwSize > 0)
{
_try
{
NewPointer = malloc(dwSize);
NewPointer = memset (NewPointer, 0, dwSize);
}
_except (EXCEPTION_EXECUTE_HANDLER)
{
}
....
}
else
{
_try
{
NewPointer = malloc(dwSize);
NewPointer = memset (NewPointer, 0, dwSize);
}
_except (EXCEPTION_EXECUTE_HANDLER)
{
}
}
return NewPointer;
}Независимо от значения переменой dwSize, всегда выполняется одинаковый код. Так зачем его дублировать?
V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 438, 442. sci_sorder.c 442
int sci_sorder(char *fname, void* pvApiCtx)
{
....
if (iRows * iCols > 0)
{
dblTol1 = pdblTol[0];
}
else if (iRows * iCols > 1)
{
dblTol2 = pdblTol[1];
}
....
}Второе условие всегда ложно, так как если EXPR > 0, то проверять EXPR > 1 уже не имеет смысла. В этом коде явно присутствует какая-то ошибка.
Разыменование нулевых указателей и неопределённое поведение
V522 Dereferencing of the null pointer 'dataz' might take place. polylinedata_wrap.c 373
BOOL translatePolyline(int uid, double x, double y, double z,
int flagX, int flagY, int flagZ)
{
double *datax = NULL;
double *datay = NULL;
double *dataz = NULL; // <=
int i = 0;
if (x != 0.0)
{
datax = getDataX(uid);
if (datax == NULL) return FALSE;
....
if (z != 0 && isZCoordSet(uid))
{
if (flagZ) {
for (i = 0; i < getDataSize_(uid); ++i)
{
dataz[i] = pow(10.,log10(dataz[i]) + z); // <=
}
} else {
for (i = 0; i < getDataSize_(uid); ++i)
{
dataz[i] += z; // <=
}
}
}
return TRUE;
}Есть массивы datax, datay и dataz. Последний нигде не инициализируется, но при определённых условиях используется.
V595 The 'number' pointer was utilized before it was verified against nullptr. Check lines: 410, 425. scilab_sscanf.cpp 410
int scilab_sscanf(....)
{
....
wchar_t* number = NULL;
....
number = (wchar_t*)MALLOC((nbrOfDigit + 1) * sizeof(wchar_t));
memcpy(number, wcsData, nbrOfDigit * sizeof(wchar_t));
number[nbrOfDigit] = L'\0';
iSingleData = wcstoul(number, &number, base);
if ((iSingleData == 0) && (number[0] == wcsData[0]))
{
....
}
if (number == NULL)
{
wcsData += nbrOfDigit;
}
else
{
wcsData += (nbrOfDigit - wcslen(number));
}
....
}Под строку number выделили память с помощью функции malloc(), при этом до проверки указателя его несколько раз разыменовывают и передают в функцию memcpy() в качестве аргумента, что недопустимо.
V595 The 'OuputStrings' pointer was utilized before it was verified against nullptr. Check lines: 271, 272. spawncommand.c 271
char **CreateOuput(pipeinfo *pipe, BOOL DetachProcess)
{
char **OuputStrings = NULL;
....
OuputStrings = (char**)MALLOC((pipe->NumberOfLines) * ....);
memset(OuputStrings, 0x00,sizeof(char*) * pipe->NumberOfLines);
if (OuputStrings)
{
char *line = strtok(buffer, LF_STR);
int i = 0;
while (line)
{
OuputStrings[i] = convertLine(line, DetachProcess);
....
}Здесь выделяют динамическую память для переменной OuputStrings, но до проверки этого указателя, выделенную память обнуляют с помощью функции memset(), а так делать нельзя. Цитата из документации к функции: "The behavior is undefined if 'dest' is a null pointer".
Утечки памяти и незакрытые ресурсы
V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] piP;'. sci_grand.cpp 990
V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] piOut;'. sci_grand.cpp 991
types::Function::ReturnValue sci_grand(....)
{
....
int* piP = new int[vectpDblInput[0]->getSize()];
int* piOut = new int[pDblOut->getSize()];
....
delete piP;
delete piOut;
....
}Тут были допущены две серьезные ошибки. После выделения динамической памяти под массивы, очищать ее следует с помощью вызова оператора delete[], т.е. с квадратными скобками.
V773 The function was exited without releasing the 'doc' pointer. A memory leak is possible. sci_builddoc.cpp 263
int sci_buildDoc(char *fname, void* pvApiCtx)
{
....
try
{
org_scilab_modules_helptools::SciDocMain * doc = new ....
if (doc->setOutputDirectory((char *)outputDirectory.c_str()))
{
....
}
else
{
Scierror(999, _("...."), fname, outputDirectory.c_str());
return FALSE; // <=
}
if (doc != NULL)
{
delete doc;
}
}
catch (GiwsException::JniException ex)
{
Scierror(....);
Scierror(....);
Scierror(....);
return FALSE;
}
....
}В некоторой ситуации выполняется выход из функции без очистки указателя doc. Также сравнение указателя doc с NULL не является корректным, т.к. если оператору new не удается выделить память, то он выбрасывает исключение, а не возвращает NULL.
Это самый наглядный пример утечки памяти, найденный в проекте Scilab. Видно, что память планируют освобождать, но в одном месте забыли это сделать.
В целом, в проекте найдено очень много утечек памяти: указатели просто не очищаются и нигде не сохраняются. Т.к. я не являюсь разработчиком Scilab, то мне сложно определить, где в таких случаях ошибки, а где нет. Но я склонен считать, то утечек памяти очень много. Наверняка мои слова могут подтвердить пользователи этого математического пакета.
V773 Visibility scope of the 'hProcess' handle was exited without releasing the resource. A resource leak is possible. killscilabprocess.c 35
void killScilabProcess(int exitCode)
{
HANDLE hProcess;
/* Ouverture de ce Process avec droit pour le tuer */
hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, ....);
if (hProcess)
{
/* Tue ce Process */
TerminateProcess(hProcess, exitCode);
}
else
{
MessageBox(NULL, "....", "Warning", MB_ICONWARNING);
}
}Утечка ресурса. Согласно документации, после вызова функции OpenProcess необходимо звать функцию CloseHandle.
Заключение
На данный момент на официальном сайте Scilab стабильной версией считается Scilab 6.0.0, но как мы успели заметить, до стабильности тут далеко. Хоть анализатором и проверялась самая последняя версия из репозитория, как правило, ошибки живут в коде очень давно, попадая и в якобы «стабильные» версии. Сам я тоже был пользователем Scilab, но задолго до того, как смог увидеть, сколько там ошибок. Надеюсь, что такой софт не сильно тормозит исследования людей, использующих подобные инструменты для математических расчётов.
Следующим проверенным проектом, в котором много математики, и он востребован в разных исследованиях, будет OpenCV library.
Примечание коллеги Андрея Карпова. Тема этой статьи сильно пересекается с мыслями, которые я излагал в статьях:
Возможно читателям будет интересно ознакомиться и с ними.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Headache from using mathematical software
Комментарии (44)
iroln
26.06.2017 12:07+3Проверка мат-пакетов статическим анализатором — это, конечно, хорошо. Но вы проверяете только C/C++ ядро. А сколько там кода, написанного на языке самого пакета (Scilab, MATLAB и т. п.)? Этот код вы проверить своим анализатором не сможете, потому что он динамический и интерпретируемый. И знаете, этот код ужасно написан, хотя бы с точки зрения качества кода и сколько там математических и логических ошибок можно только гадать.
Недавно ковырялся в официальном матлабовском коде
Заголовок
Arastas
26.06.2017 12:26Простите, я не очень понял. Какие претензии к тому, что подчеркнуто красным? Кроме жутких имён переменных, которые за матлабом тянутся с 80-х?
iroln
26.06.2017 12:31Дублирование кода. Вычисление матрицы QtWQ и вычисление выражение для u можно вынести из под условия if else. Условие тут нужно только для вычисления параметра p.
Там весь код так написан, очень сложно читать.
netch80
26.06.2017 13:17+3QtWQ в обеих ветках вычисляется одинаково. Поэтому лучше его вынести в безусловное вычисление за пределы развилки.
После того, как это сделано, оказывается, что оба вычисления для u имеют одинаковую форму:
u=((6*(1-p)*QtWQ+p*R)\diff(divdif)
и «else» вообще не нужна, а «then» просто меняет значение p; вычисление u может быть сделано общим кодом после завершения развилки.
Мне лично кажется, что после этих изменений код будет понятнее и сопровождабельнее.Arastas
26.06.2017 14:18Я, конечно, не могу залезть в голову разработчикам, но попробую предположить, почему сделано так как сделано.
Итак, как я подозреваю, речь об аппроксимации (сплайны?), а Qtw имеет специальный тип, описывающий разреженные матрицы (вызов spdiags). Причём, размерность n на n, где n, веорятно, это число точек кривой.
Если мы идём по ветке if, то приозведение Qtw*Qtw используется дважды — при вычислении p и при вычислении u. В этом случае разумнее избежать повторного очень дорогостоящего вычисления произведения и заранее его сохранить в QtWQ. Если же мы идём по ветке then, то нет необходимости повторного использования произведения, и мы экономим на создании в памяти ненужного (но большого!) QtWQ.
Таким образом, незначительно теряя в читаемости, мы выигрываем в производительности и без того ресурсоёмкой задачи.netch80
26.06.2017 14:39+2> выигрываем в производительности и без того ресурсоёмкой задачи.
Выигрыша нет, QtWQ безусловно и одинаково вычисляется в обеих ветках. Если написано Qtw*Qtw.', это оно же.
Или Вы предполагаете, что в else реально происходят «ленивые вычисления» и работа с QtWQ «размазывается» на остальном? Такое я про матлаб пока не слышал, хотя готов поверить, если подтвердят.
iroln
26.06.2017 14:43+2QtWQ нужно вычислять в любом случае, дублируя код мы ни в чём не выигрываем.
Заметьте ещё "однострочники" через точку с запятой. Ради чего тут экономия строк? Это трудно читаемо и усложняется отладка с помощью отладчика.
Arastas
26.06.2017 15:43Я не говорил, что не производятся вычисления. Я говорил, что экономится время на создание новой переменной. Одно дело посчитать значения в ходе вычислений, другое дело создать новую переменную специального типа весьма большой размерности. Тем более, этот код, если я не ошибаюсь, многократно вызывается из цикла.
Я сделал небольшой бенчмарк по построению сплайнов для 10 тысяч точек. Разница совсем небольшая, порядка 0.1 мс на вызов, но предложенная netch80 версия достоверно оказывается медленнее. В доументации пишут, что первая версия этой функции была сделана в 1987. Я не знаю, когда именно был добавлен обсуждаемый участок кода, но у меня есть существенная уверенность, что на тот момент выигрыш в производительности был куда как более заметен.
Да, Matlab известен своим печальным оформлением кода (хотя, если Вы откроете другой код 80ых, то там, возможно, будет что-то похожее. Надо Fortran посмотреть). Но не нужно "негативный ореол" оформления автоматически переносить и на быстродействие/оптимизированность кода. Хотя да, проблемы, как и у всех крупных проектов, бывают.
iroln
26.06.2017 15:56Этот код в цикле не вызывается, матрицы там разреженные, поэтому то, о чём вы говорите — это какая-то уж совсем микрооптимизация получается. 0.1 мс против читаемости и поддерживаемости кода — это уже слишком, учитывая, что в матлабе вообще всё передаётся по значению (кроме handle based классов). Хотя о чём это я, ООП в матлабе в десятки раз медленее чем обычные функции. :)
В доументации пишут, что первая версия этой функции была сделана в 1987
Изначально этот код был написан на фортране Карлом де Буром для книги. Думаю, что первые версии матлабовского кода написаны им же.
Но не нужно "негативный ореол" оформления автоматически переносить и на быстродействие/оптимизированность кода
На матлабе тоже можно писать нормально, просто это никто не делает, потому что большинство пишут на матлабе write-only "наколенчатый код" лишь бы проверить работу модели или алгоритма.
Arastas
26.06.2017 16:18-2Вот смотрите. Я пишу
первая версия этой функции была сделана в 1987
есть существенная уверенность, что на тот момент выигрыш в производительности был куда как более заметен.
а Вы мне отвечаете0.1 мс против читаемости и поддерживаемости кода — это уже слишком
В целом, я не могу понять Ваше тезис. Вы хотите сказать, что разработчики в 1987 году написали более сложный и хуже читаемый код без всяких оснований? Или что спустя 20-30 лет они должны были переписать существующий и работающий код, так как на новом железе выигрыш по времени стал слишком маленьким?iroln
26.06.2017 16:41+3Последний раз этот код правился в 2010 (судя по документации). Я считаю, что код нужно писать так, чтобы он был понятен и легко поддерживаемым. Периодически надо делать рефакторинг. Это не тот случай когда надо упарываться по поводу создавать или не создавать переменную. Мы не живём в 1987 и на фортране
уже почти никто не пишет(на самом деле пишут, но не в этом суть). Просто сидят вот такие математики и колбасят вот такой код. Я работал в двух компаниях где практиковалось такое программирование алгоритмов. И каждый раз подобный код простовыкидывалсяне использовался и писался заново, чтобы потом написать на его основе понятный C++-код для использования в production.
Объясняю свою мысль в контексте этой статьи: подобный код сложно читать и понимать, а значит в нём легко могут затаиться ошибки, которые трудно заметить. И бывает, что подобные ошибки, закрадывающиеся в формулы, даже тестами не отлавливаются.
iroln
26.06.2017 14:48+4Вообще, я понимаю, почему вы "защищаете" этот код. У вас в профиле написано, что вы работаете с Matlab. Для вас этот код выглядит совершенно нормально.
Я не перестаю писать о том, что код на Matlab ужасен в 99% случаев, потому что он написан математиками для "калькулятора", каким они считают компьютер, а не для людей.
Вот мой комментарий на эту тему:
https://habrahabr.ru/post/324052/#comment_10121034
SvyatoslavMC
26.06.2017 14:25+1Этот код вы проверить своим анализатором не сможете, потому что он динамический и интерпретируемый.
Вряд ли дело в этом :D Скорее у нас просто нет анализатора для этого языка.iroln
26.06.2017 14:40Вряд ли дело в этом :D Скорее у нас просто нет анализатора для этого языка.
Именно так :) Ну и в том числе ловить ошибки статическим анализом в языках с динамической типизацией сложнее. Сделайте хороший статический анализатор для Python, которому не нужны аннотации типов, я
вас расцелуюкуплю его. :)
Wano987
27.06.2017 13:10Кстати о птичках: будет ли поддержка иных языков? Тот же РНР, например, будь он не ладен.
amarao
26.06.2017 13:57По поводу кривого алгоритма. У них там тестов нет что ли? Совершенно же тривиально тестируется.
PTM
26.06.2017 14:00+1А openFOAM не проверяли?
SvyatoslavMC
26.06.2017 14:28Нет, но звучит знакомо. Возможно давно кто-то из коллег пытался проверить, но не было технической возможности это сделать. Могу добавить в список на проверку.
KF-121
27.06.2017 22:40не могли бы вы добавить в список проверки ОСРВ Arinc 653 совместимую разрабатываемую ИСП РАН. ссылка на git
SvyatoslavMC
27.06.2017 23:50Надеюсь они не сильно обидятся после этого, ведь сами занимаются статическим анализом :D
Лучше добавляйте сразу сюда. И Вы ( PTM, veprbl ) тоже. Не обращайте внимание, что там накопились Pull Request'ы. Я потихоньку их просматриваю и применяю.KF-121
28.06.2017 09:16ну вот как раз и проверить насколько хорошо их инструмент работает и используют они его при работе ))
Soarer589
26.06.2017 14:19+3Не далее как неделю назад мозг ломал. В зависимости от порядка загрузки файлов, работы с меню или ещё каких-либо совершенно левых действий, Scilab на одной и той же программе выдаёт четыре результата:
1) нормальная работа
2) ошибка на определённой строке
3) подвисание программы
4) вроде нормальная работа, но конечный график — нули
Грешным делом подумал, что глюки 64-разрядной версии. Поставил 32-разрядную. Глюки те же.
А вон оно чё, Михалыч, оказывается…
borisxm
26.06.2017 14:19+3Здесь вы, к сожалению, правы. Качество C/C++ кода во многих математических библиотеках очень низкое. Причем практически безотносительно к их платности или бесплатности. Нередко все заканчивается переписыванием функций с нуля, что терпимо для открытого кода, но сильно раздражает в платном софте.
SvyatoslavMC
26.06.2017 15:43+1От разработчиков узнал, что они используют Coverity Scan: scilab. Похоже кто-то не так крут, как принято считать.
encyclopedist
26.06.2017 17:24+4Ну там в репорте видно, что и Коверити находит у них уйму проблем. Так что дело не в анализаторе, а в разработчиках и их ресурсах.
Ilias
26.06.2017 18:01+3мне только одно не понятно — ошибка в умножении матриц, это как вообще? этой библиотекой вообще не пользуются?
SvyatoslavMC
26.06.2017 18:19Point Cloud Library, по которому и нашлась библиотека в зависимостях. Вроде делают что-то серьёзное и как-то живут с такими ошибками.
alexeng
26.06.2017 18:01-1MC-21 многое сделано на matlab/simulink. Военный Сухой делает скв и сард в нашем софте — SimInTech. Рассматривают его использование для остальных систем. Гражданские самолёты Сухого — matlab/simulink. Причём походу это не свой осознанный выбор, а так как ЦАГИ для них что-то делает. Ил размышляет. Но некоторые стенды уже с SimInTech. И да, выбранный софт никак не влияет на качество аппарата, там действительно большое количество испытаний, скорее на его стоимость.
ivlis
26.06.2017 21:18Если в тулбоксе нет тестов, которые проверяют умножается ли матрица на число корректно, то им не стоит пользоваться.
По поводу этой ошибки, скорее всего вы нашли какую-то заброшенную часть кода, которая никогда не выполняется. Иначе бы эту багу уже давно заметили, уж слишком тривиально и ведет уж к слишком другому результату. Ну или этим пакетом просто никто не пользуется.
veprbl
27.06.2017 05:50+1Про https://root.cern.ch интересно было бы почитать. Там своя копия llvm, её можно особо не проверять. Пожалуй, наиболее важные компоненты будут: core, io, tree, hist и math. Я так понимаю у них там уже используется Coverity.
n_ilyin
27.06.2017 09:09+2Немного оффтопа: в научной школе М.А. Миллера по поводу ошибок была в ходу такая шутка-спор: Один говорил: «Я возьму любую диссертацию, открою на случайно странице и найду ошибку». Как правило оппонент тушевался и не знал что отвечать. Правильный ответ: «А я найду две!»
Так и живем.
impetus
У меня товарищ в пакете StarCD считал как-то довольно важную клыльчатку… Потом переставил систему (кажется с 2000 на XP) и продолжил. Результаты получились другие. Прогнал тестовые — совпадают. Прошерстил настройки, почитал доки — так и не понял где засада.
Крыльчатка в ушла в производство в тираж, но «с тех пор в себе я сомневаюсь».
SvyatoslavMC
Я сейчас переживаю за новый российский самолет МС-21. Мало ли в каком софте его проектировали по программам импортозамещения.
mezastel
Тут выход простой — не летайте на нем. Пускай как SSJ поставляют его а страны 3 мира.
impetus
Вот тут — https://ru.wikipedia.org/wiki/Список_самолётов_Sukhoi_Superjet_100 список эксплуатирующихся SSJ. Негусто там со странами 3-го мира. Пожалуйста, не надо тащить на Хабр политику! — для этого есть почти весь остальной рунет.
impetus
К счастью авиация как отрасль софту априори не доверяет, проверяя всё в обязательном порядке экспериментами — продувают в трубах, ломают на стендах… Программа сертификации потом — в натуре надо подтверждать полётами все-все возможные случаи (SSJ напр в Кефлавике пострадал — сертификационный полёт — взлёт на одном двигателе при сильном боковом ветре (что-то там пошло не так — не суть, суть что реально физически летают все тесты и потом дефектоскопируют подробнейше)). Вот с импортозамещёнными хим.компонентами нового композитного крыла — у неспециалистов есть опасения, но это тема целого отдельного холивара.
KF-121
имею некое отношение к мс-21, могу сказать, что на одном из подразделений используются для проектирования 3д — NX (ранее «Unigraphics»), 2д — nanocad. генерация кода — SCADE.
возможно используются и другие генераторы. Но по своей специфике работы скажу(тестирование авиационного ПО), что каждый используемый код генератор квалифицирован по do-178b/c.
KF-121
к данному коду не применяются такие активности «код ревью», соответствие код стандарту. но тестирование кода проходит полное и только на основе требований с сбором структурного покрытия кода по уровням DO-178C Level A/B/C для определения неописанного или ошибочного кода.
SvyatoslavMC
Сейчас просматриваю огромную базу CWE с примерами. Иногда такая чушь попадается, а ведь кто-то под это дело сертификаты выдаёт. Отсюда у меня такое недоверие ко всякого рода стандартам с длинными аббревиатурами. Профессиональная деятельность оставляет отпечаток на мировоззрении.
Soarer589
Осмелюсь предположить, что засада была связана с использованием каких-то системных функций, выполняющих перевод числа в экспоненциальной форме из символьной строки в собственно числовой тип и/или наоборот.
Довелось тут запускать древний Electronics Workbench 5.12 под Windows 10. Результат моделирования выдавался абсолютно неадекватный. Собираю цепь — источник переменного напряжения, и простейший выпрямитель — диод плюс резистор. Напряжение на резисторе выходит переменное. Мда… Когда второй раз полез в опции анализа, оказалось, что все значения вида 1E-13 стали нулями. Где уж тут нормально считать! В принципе, оно было решаемо записью вида 0.0000001 в поле (на сколько нулей ширины поля хватало). Но была же ещё и куча таких записей в моделях элементов. Всё не переделать. В общем проблема решилась запуском в режиме совместимости то ли с семёркой, то ли с XP.