Checking with PVS-Studio static analyser

[SviatoslavRazmyslov]

To demonstrate the capabilities of our analyzer, we regularly perform analysis of open source projects. We had recently checked the Far Manager project.

Here is the link to the article about it: Porting is a Delicate Matter: Checking Far Manager under Linux

If you have any questions, or if you are interested in the evaluation of our static analyzer or in any other source code quality control services that our company provides, please contact us at [email protected].

[lieff]

Here in russian https://habrahabr.ru/company/pvs-studio/blog/321948/

[elfmz]

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

[elfmz]

Решил поковырять немного..

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

• /V567 Undefined behavior. The '::ViewerID' variable is modified while being used twice between sequence points. viewer.cpp 117/
  А вот..нет. Слева и справа тут стоят две разные переменные, одна - глобальная, другая - член класса, согласно области видимости. Сам в шоке, обычно такое различие делают через this->, авторы фара соригинальчичали конечно, но однако же проверил - все ок. Две совершенно разные переменные.

Но надо признать есть и реальные косяки.

[SviatoslavRazmyslov]

@elfmz про неопределённое поведение довольно точные диагностики, я ещё раз посмотрю.

Багов тут нет, просто неаккуратненько..

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

Но надо признать есть и реальные косяки.

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

[elfmz]

Варнинги на APIStringMap.cpp прямиком из кода wine - dlls\kernel32\locale.c, даже не знаю что с ними делать. С одной стороны if-ы вроде как "сокращаются", с другой - если их "сократить" то код станет менее красивым и читабельным, так что пожалуй оставлю как есть :)
Кстати вот wine тоже стоило бы потрясти.

[ghost]

@elfmz

А можно глупый вопрос? Зачем там используется Wine, если это полноценный порт?

[SviatoslavRazmyslov]

Кстати вот wine тоже стоило бы потрясти.

Уже 2 раза трясли (тут, тут), но у нас уже очередь. Разрабатываемые проекты можно трясти вечно, отсюда вывод - интеграция и регулярный анализ есть верный сценарий использования.

[elfmz]

А можно глупый вопрос? Зачем там используется Wine, если это полноценный порт?
Собственно функции работы с кодировками WideCharToMultibyte/MultibyteToWideChar строк целиком взяты из wine. А насчет 'зачем именно из wine' - а почему нет? Можно конечно переделать на libiconv, но зачем?

[ghost]

Собственно функции работы с кодировками WideCharToMultibyte/MultibyteToWideChar строк >целиком взяты из wine. А насчет 'зачем именно из wine' - а почему нет? Можно конечно переделать >на libiconv, но зачем?

тогда всё понятно.. еще как вариант на будущее... можно попробывать в ReactOS посмотреть...

[unxed]

ReactOS сами импортируют из вайна всё, что могут по части юзерспейса, а свои улучшения шлют потом обратно. Так что смысла нет.

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

[elfmz]

Оказалось что фикс PVS варнинга ( 748c143 ) вызвал баг #264
Вобщем, как было вначале - было правильно. sizeof(void *) в SetUserData хэндлится особым, не очевидным с первого взгляда, образом.

[pavel-pimenov]

https://www.fly-server.ru/pvs-studio/far2l/