Comments 19
UFO just landed and posted this here
Переменная data используется перед проверкой на null, что потенциально может привести к возникновению исключения NullReferenceException. Если же переменная data никогда не является нулевой (null), то расположенная ниже проверка бесполезна и её следует удалить, чтобы она не вводила в заблуждение.
Там идет не проверка на null, а на IsNullOrEmpty. Удалять ее нельзя, но можно заменить на != ""
UFO just landed and posted this here
Самый первый баг точно приводил к падениям при просмотре истории пустого репозитория в какой-то версии. Потом, вроде бы, перестал — но, по всей видимости, вместо исправления заткнули костылем.
ну, вот именно «как-то» )))
либо пользователи просто с этим мирятся, либо этот код мало когда выполняется, т.к. никто не использует продукт в таких условиях
например, в самом Git'е есть «кросс-конвертирование» кодировки сообщения коммита: когда написанное в UTF-8 сообщение нормально отображается на не-UTF-8 терминалах, и наоборот; настройки
но в какой-то момент у меня это не сработало, а всё потому, что я использовал не просто
и таких примеров — масса
З.Ы. пришлось самому исправлять и отправлять патчи
либо пользователи просто с этим мирятся, либо этот код мало когда выполняется, т.к. никто не использует продукт в таких условиях
например, в самом Git'е есть «кросс-конвертирование» кодировки сообщения коммита: когда написанное в UTF-8 сообщение нормально отображается на не-UTF-8 терминалах, и наоборот; настройки
i18n.commitencoding и i18n.logoutputencoding)но в какой-то момент у меня это не сработало, а всё потому, что я использовал не просто
git log ..., а git log --format в таких репозиториях…и таких примеров — масса
З.Ы. пришлось самому исправлять и отправлять патчи
Для статического анализа свойственно обнаруживать ошибки в редко- или даже никогда не выполняющемся коде. Именно поэтому такой код «как-то работает». Однако, в любой момент в исходник могут быть внесены изменения, после которых такой код станет часто используемым. При этом выяснение причин возможных неполадок будет затруднено. Именно поэтому полезно использовать именно регулярный статический анализ. Чтобы потом не искать ошибки в коде, написанном 3 года назад, да еще почти не использовавшемся.
Статический анализ следует использовать не только из-за этого. Да, многие ошибки в новом коде быстро исправляются при первичном тестировании. Но можно найти часть багов не в отладчике, зря тратя время, а вообще сразу ещё на этапе написания кода с помощью SCA.
Просто хотел напомнить мысль, что самое главное это возможность найти новые баги при написании кода, а не в поиске багов в редко используемых местах.
Просто хотел напомнить мысль, что самое главное это возможность найти новые баги при написании кода, а не в поиске багов в редко используемых местах.
del
Помоему это признание когда о твоем продукте пишут авторы PVS-Studio :-)
Спасибо за проверку, будем исправлять :-)
Так и есть!
Кстати с Debug.WriteLine("Loading plugin...", pluginFile.Name); это к сожалению не баг, а дурной дизайн API Debug.WriteLine.
Есть перегрузка метода принимающая на вход две строки https://msdn.microsoft.com/ru-ru/library/1w33ay0x(v=vs.110).aspx
В выражении ищется более длинная (notepad++) и более короткая (notepad) подстроки. При этом проверка с более длинной строкой никогда не сработает, так как проверка с поиском более короткой строки будет ее предотвращать. Как я уже упомянул, это не ошибка, а всего лишь опечатка, но в другой ситуации невинная избыточная проверка могла бы превратиться в потенциально коварный баг.
Не согласен с тем что это ошибка или опечатка. Если исправить как предлагается и вдруг захотят убрать проверку на «notepad» то с большой вероятностью перестанут работать оба варианта. Всякое бывает, и не всё задокументировано.
Небольшая избыточность кода которая лучше описывают логику работы и помогает поддерживать код — не баг и не опечатка. К тому же в данном случае проверки записаны в правильном порядке и не влияют на производительность. Какие-нибудь умные компиляторы/анализаторы, в теории, даже выкинуть смогут этот лишний код при компиляции.
UFO just landed and posted this here
Автор предлагает убрать проверку на notepad++ как не имеющую смысла с точки зрения выполнения программы. Но она, скорее всего, имеет смысл для разработчиков.
Если поменять местами то как раз таки появится лишний вызов поиска подстроки и тогда можно было бы писать про ошибку. А так всё хорошо в этом фрагменте кода.
Если поменять местами то как раз таки появится лишний вызов поиска подстроки и тогда можно было бы писать про ошибку. А так всё хорошо в этом фрагменте кода.
Просто представьте, что для «notepad» и «notepad++» потребуются разные обработчики. И как только вы их туда добавите, так сразу и обнаружится, что почему-то обработчик для «notepad++» почему-то вообще не вызывается, а вместо него вызывается обработчик для «notepad»… Так что правильный порядок — от длинной строки к короткой…
Кстати, вот этот pull-request целиком состоит из исправлений, найденных триальной версией PVS-Studio. Тогда ещё было найдено несколько багов в PVS-Studio, а также было выяснено на своём опыте, что человек (особенно в пятницу вечером) может не видеть ошибки там, где видит и указывает анализатор, которому не сложно работать не уставая!
Sign up to leave a comment.
Ищем и анализируем ошибки в коде GitExtensions