New #9
New #9
Conversation
|
Хотелось бы еще поинтересоваться насчет Вашей терминологии: какая мотивация использовать слово "критерий"? В документации oclint`а используется термин "rule", прямой перевод -- "правило", никак не "критерий" |
|
Против слова "правило" ничего не имею, использовал "критерий" просто потому, что не сильно заглядывал в код. |
tester.py
Outdated
| makefile_exists = False | ||
|
|
||
| content = os.listdir() | ||
| for x in content: |
There was a problem hiding this comment.
Предлагаю убрать логику "понимания способа сборки" и заменить ее на флаг командной строки (эта логика у нас уже есть в вышестоящих скриптах).
tester.py
Outdated
|
|
||
| os.chdir('..') | ||
| os.remove('compile_commands.json') | ||
| os.system("rm -rf .tester_tmp") |
There was a problem hiding this comment.
Как правило, для цепочек скриптов в качестве способа передачи информации о корректности / некорректности работы используют коды возврата. Добавьте в вашем с скрипте выход с ненулевым кодом, если были найдены ошибки в исходниках (с точки зрения линтера) или произошли любые другие нештатные ситуации.
К нештатным ситуациям я бы отнес любые сбои при запуске внешних команд (это будет такая защита на случай, если мы криво настроим вашу утилиту в рабочем окружении). Поэтому, стоит добавить обработку исключений.
tester.py
Outdated
| os.system("mv compile_commands.json ../") | ||
|
|
||
| for file in files: | ||
| os.system(f'/bin/sh -c "oclint --rule=TooLongIfSequence "{file}" | grep etu"') |
There was a problem hiding this comment.
Я бы вынес в параметры командной строки данного скрипты аргументы для oclint (список используемых правил) - так он станет более гибким.
grep предлагаю убрать - лучше его заменить на обобщенный анализ фидбека от линтера (есть замечания - выводим их как есть + трубим тревогу, нет замечаний - пишем об этом и выходим спокойно)
There was a problem hiding this comment.
Коменнтарии (+ часть ещё в коде):
- Pull-request'ы желательно называть +- осмысленно, чтобы понятно к чему он относится
- На вики про сборку желательно привести полную команду по установке всего для Ubuntu (как одного из самых попрулярных дистрибутивов)
- При пересборке почему-то не подтянулись уже собранные объектники
- В инстуркции явно не хватает подробностей о том, как нужно поставить
oclintи куда (и нужно ли это делать, потому что судя по скриптам -- нет, а на самом деле да и нужно прочитать инструкцию по установке). - Использование
bearне такая большая проблема, тесты же как-то прогоняются == программа как-то собирается. Вот в этот момент можно вызвать bear. - Будет ли работать с компилятором
gcc, которые используется в проверках (я может быть потом проверю самостоятельно)
В остальном выглядит хорошо, оно работает, есть понимание как делать дальше
build-oclint.sh
Outdated
| if [ ! -d oclint ]; then | ||
| git clone https://github.com/oclint/oclint.git oclint | ||
| cd oclint | ||
| git reset --hard d776db51c8574df406b2b0dc1b43b0b9b2d86d34 |
There was a problem hiding this comment.
Хэш коммита вынести в отдельную переменную
install-oclint.sh
Outdated
| @@ -0,0 +1 @@ | |||
| cp -r oclint/build/oclint-release/lib/* /usr/local/lib/ | |||
There was a problem hiding this comment.
Можно добавить аргумент -- куда конкретно устанавливать
There was a problem hiding this comment.
Заодно нужно это добавить в CMakeLists.txt, чтобы там тоже можно было кастомизировать (или предоставить пример, как это сделать, я сходу не помню все возможности cmake)
There was a problem hiding this comment.
Тесты надо оставить, это хорошая практика, позволяет проверять, что ничего из работающего раньше не сломалось
rules/TooLongIfSequenceRule.cpp
Outdated
| using namespace clang; | ||
| using namespace oclint; |
There was a problem hiding this comment.
Не самая хорошая практика подключать namespace, потому что:
- Повышается вероятность пересечений имен
- Становится менее интуитивно понятно, откуда та или иная функция
Можно подключать namespace, если это часть используемого проекта для упрощения или есть вложенные namespace, чтобы сократить обращение. std обычно не подключают (из быстрого нашел такое: https://stackoverflow.com/questions/2712076/how-to-use-an-iterator/2712125#2712125)
rules/TooLongIfSequenceRule.cpp
Outdated
| size_t getIfComplexity(IfStmt *if_stmt) | ||
| { | ||
| Stmt *else_stmt = if_stmt->getElse(); |
There was a problem hiding this comment.
Желательно заменить на цикл, с рекурсией все может быстро закончится (а chatgpt или copilot могут нагенерировать миллион if-else statements )
rules/TooLongIfSequenceRule.cpp
Outdated
| @@ -80,7 +85,7 @@ class TooManyConsecutiveIfStatementsRule : public AbstractASTVisitorRule<TooMany | |||
| else { | |||
rules/TooLongIfSequenceRule.cpp
Outdated
| { | ||
| private: | ||
| int max_if_sequence_len; | ||
| size_t getIfComplexity(IfStmt *if_stmt) |
There was a problem hiding this comment.
Если я правильно понял, то это данный критерий: #4 . По коду я вижу, что проверяется только последовательность if'ов, но не проверяется, что они похожи (я слепой, поэтому если чего-то не увидел, то мне нужна явная строчка). Это разные проверки, так как бывают функции, которые проверяются корректность данных и в них может содержать много последовательных if, но они все проверяют разные условия, и это нормально (ненормально, если они в основной функции, но это уже совсем другая история)
There was a problem hiding this comment.
Пример кода:
#include <stdio.h>
int main()
{
int x = 0;
int param1 = 0;
float param2 = 0.0;
float param3 = 0.0;
float param4 = 0.0;
int opt = 0;
int value = -1;
scanf("%d %d %f %f %f %d %d", &x, ¶m1, ¶m2, ¶m3, ¶m4, &opt, &value);
if (x == 0) {
puts("Zero");
}
if(param1 > 100){
puts("Too large");
}
if(param2 < 0.0){
puts("Negative");
}
if(param3 * param2 > 1e+5){
puts("Invalid");
}
if(param4 / 2 < param1){
puts("Unique case");
}
if(opt < 0 || opt > 6){
puts("Invalid option");
}
if(value == -1){
puts("By default");
}
return 0;
}Проверки разные и все важны, но при данной реализации проверки, будет предупреждение
tester.py
Outdated
There was a problem hiding this comment.
Это хорошо, что вы подумали заранее об удобном способе запуска. Однако есть несколько моментов:
- Что будет, если не использовать
bear(соответсвенно не использоватьcompile_commands.json), а анализировать файлы по отдельности? Вроде бы, мы пока проверяем стандартные конструкции языка, которые должны работать и без дополнительных файлов - У нас в коде самих проверок уже есть определение способа сборки. Добавление данной функциональности сюда выглядит как дублирование кода, которое придётся дополнительно поддерживать + делает менее применим данный скрипт
- Выглядит так, что данный скрипт делает следующее: 1) определяет способ сборки; 2) собирает проект с захватом
bear; 3) парсит вывод; 4) Подчищает какие-то лишние файлы . Из трёх пункто непосредственно к утилите относятся только 2 и 4 (1 -- это отвественность системы проверки, 3 -- какие варнинги важны, а какие нет тоже определеяет система проверка, назначение утилиты их подсветить, если они есть). Получается, что можно упростить до следующего: собрать проект с захватомbearпри помощи командойcmd, которую подаст пользователь; удалить лишние временные файлики. Кажется, для этого не нужно python и можно обойтись стандартными командами bash для всего этого
There was a problem hiding this comment.
- Что будет, если не использовать
bear(соответсвенно не использоватьcompile_commands.json)
Размышляя над этим вопросом, пришел к двум выводам:
- В текущей реализации
tester.pyне проверяет подключаемые заголовочные файлы (а в них тоже можно написать что-нибудь "запрещенное"); - Критерии, как вы и сказали, действительно не требуют ничего сверхъестественного.
Поэтому вместо bear использовать oclint напрямую для каждого .c/.cpp/.h файла звучит как хороший вариант. Но здесь, как мне кажется, возникают другие проблемы:
- В одной из лабораторных мне пришлось воспользоваться
-std=c99. Причину уже не помню. Может ли oclint, не зная про этот флаг, совершить какую-нибудь ошибку? Стоит это проверять? - Студенты могут поэкспериментировать и прийти к выводу, что
.cфайл с русскойсне проверяется системой ( Гомоглифы Юникода). Сbearтакой проблемы не возникнет, имена файлов он прослушает правильно. Безbearвижу решение только в сообщении:Ошибка: посторонние файлы в src.
Может это и не проблемы вовсе, но это было первым, что прошло в голову.
README.md
Outdated
| Вся информация в wiki. |
There was a problem hiding this comment.
В README нужно хотя бы оставить простейшую инструкцию (без деталей) как собрать и запустить. Для деталей можно оставить ссылку на конкретные страницы на вики
|
Чуть позже постараюсь прокомментировать и дополнить. |
|
Что сделано:
При пересборке чего именно? Если при пересборке кастомных критериев, то не знаю в чем дело. Если же при пересборке самого OCLint, то:
У меня работает (Debian 12). Остальное не могу знать. |
|
|
GlobalVariableRule отлавливает глобальные и static переменные |
|
|
Создал один большой Makefile, заменяющий все .sh скрипты для установки и сборки всего подряд. Про него можно почитать в обновленном wiki. README.md тоже был обновлен |
|
Доделанный TooLongIfSequenceRule Проверяет подряд идущие if-ы не только на их существование, но и смотрит, какие у этих if-ов условия. Сравнивая условия, он просто проходится по их синтаксическим деревьям. Степень "жесткости" проверки можно настраивать. Этот критерий имеет следующие параметры: MAX_IF_SEQUENCE_LEN (по умолчанию равен 5) - это максимальная разрешенная длина последовательности ифов с похожими условиями CHECK_BINARY_OPERATOR (по умолчанию равен 1) - если равен единице, тогда критерий будет смотреть на знаки в условиях if-ов. Например, "x < 5" и "x > 5" для него будут разными условиями. Чтобы выключить такое поведение, надо присвоить этому параметру значение 0. CHECK_FUNCTION_CALL (по умолчанию равен 1) - если равен единице, тогда критерий будет смотреть на то, какие функции вызываются внутри условий if-ов. Например, "gcd(x, y)" и "lcm(x, y)" для него будут разными выражениями и условия, их содержащие, будут тоже считаться непохожими. Чтобы выключить такое поведение, надо присвоить этому параметру значение 0. Напомню, как передавать параметры OCLint`у: через флажок -rc: т.е. можно писать так: |
|
Не смог запустить Вроде всё делал по инструкции, а сборка сломалась |
build-oclint.sh клонирует репозиторий oclint, сбрасывает его до версии 22.02 (последняя версия на данный момент) и запускает скрипт сборки oclint`а. Это нужно, чтобы собрать нужные файлы для последующей сборки СВОИХ кастомных правил.
CMakeLists.txt -- франкенштейн из файлов CMakeLists.txt oclint`а. Некоторые части были немного переписаны под нужды проекта, некоторые были слепо скопированы. Работает => не трогаю.
tester.py принимает на вход путь до папки с исходным кодом лабораторной/курсовой работы студента (Ivanov_Ivan_cw/src). Сначала скрипт определяет способ сборки. Доступны способы из блока проверки курсовой на e.moevm:
Далее в папке с исходным кодом работы скрипт создает папку для временных файлов. С этого момента скрипт работает только в ней (для чего это нужно, напишу в самом конце (!!!)). Все файлы копируются во временную папку, после чего в ней запускается bear -- утилита для генерации compile_commands.json. Без этого JSON файла oclint будет проверять все файлы по-отдельности без какого-либо контекста, это нам не нужно.
Также правило TooManyConsecutiveIfStatements было мной переименовано в TooLongIfSequence, потому что так банально короче. Теперь можно изменить MAX_IF_SEQUENCE_LEN (по умолчанию равно 5). Подробнее про конфигурацию oclint`а здесь
(!!!) Утилита bear полноценно СОБИРАЕТ проект со всеми вытекающими: объектные файлы, исполняемый файл, потраченное ВРЕМЯ. Если я правильно понял, изменить это никак нельзя. Если это критично, возможно стоит отказаться от bear и воспользоваться этим:
https://stackoverflow.com/questions/21134120/how-to-turn-makefile-into-json-compilation-database
Этот код, похоже, не будет ничего собирать, однако доверия он не вызывает.
Хотелось бы услышать комментарии и критику, в частности критику tester.py. По ощущениям сделал грязь.