Pull to refresh

Comments 26

for (int index = 1; index < a.Length; ++index) — Во, тут ошибка!

Это баг редактора хабра. В обработке буфера копирования Markdown-кода остался код экранирования HTML-разметки. Уже месяц так.
Обычно C# программист не допускает таких ошибок потому что знает что текст обрамляется строго двойными кавычками. В дополнение IDE обычно показывает в какую перегрузку вы попали.
кто реально проверяет в какую перезагрузку попали после того как закончили с методом и IDE не показало ошибку?

Райдер кстати сам подсветил это место как подозрительное, так что даже по перегрузкам ходить не пришлось.

Возможно, обычно не допускает, но как говорится — и на старуху бывает проруха. Как и показывает пример из статьи. :)

if (a.Length > 0)

Метод публичный, где проверка на null? Анализатор такого не ловит?

Просто при анализе параметров метода — сейчас нет.


Но здесь будет межпроцедурное срабатывание, если в метод будет передано значение, которое может быть null. В таком случае анализатор выдаст предупреждение на место вызова с доп. навигацией на точку разыменования (как раз на a.Length):


object[] arr = null;
var _ = ToString(arr);

Предупреждение PVS-Studio: V3080 Possible null dereference inside method at 'a.Length'. Consider inspecting the 1st argument: arr.


Если же вернуться к разговору про анализ исключительно параметров методов, безотносительно контекста вызова метода, есть более специфичное правило — V3115. Оно срабатывает на 'Equals' (идея из гайдлайнов MS на Equals, скорее всего).


Если говорить про более общий случай — ругаться на параметры публично-доступных методов, используемых без проверки — была такая идея. Уж мне то такие срабатывания могли бы быть удобны, статьи проверку исходников библиотек писать. ;) Но при первых прогонах на базе реальных проектов, насколько я помню, их оказалось просто какое-то неимоверное число — хотелось как-то сузить. Тогда были более приоритетные задачи / полезные диагностики, так что та идея ещё жива, но пока в TODO.

Наверное, по хорошему стоит смотреть на настройку CA1062. Если разработчик его установил, то стоит ругаться, а если запретил то конечно решаться не следует.

Я тоже именно на отсутствие проверки на null обратил внимание. ReSharper умеет это отлично ловить, если задавать для параметров и возвращаемых значений его аттрибуты [CanBeNull] / [NotNull] и [ItemCanBeNull] / [ItemNotNull] У меня ими всегда весь код обклеен.
Анализатор VS выдает «CA1062: Validate arguments of public methods», но только если метод объявлен публичным. Если объявить его internal, уже не ругается. VS по сравнению с ReSharper очень скромен в этом плане. На возможную ошибку с неявной конверсией символа в int ReSharper тоже сразу начинает ругаться.

Почему сразу ошибка, может так и было задумано?

Она самая. И плюс толика сарказма, но я уже за это отхватил.

Именованные аргументы хорошо защищают от таких ошибок неявного кастинга. Сразу было бы видно «capacity: ‘[’», и сразу бы возник вопрос.
Да и вообще, нормальный проект должен быть покрыт тестами и бенчмарками. В данном случае есть подозрение, что это какой-то отладочный ToString.

Такую ошибку не словит ни один тест. Тут создается только лишняя начальная вместимость у sb. А писать именованные аргументы слишком долго (мало кто этим занимается).

Как раз такую ошибку словит любой тест. Даже самый простой:


Assert.Equals("[]", xxx.ToString(new object[0]));
Действительно. Я ошибся.
Кажется кто-то забыл протестировать код :)

sb.Append(", ").Append(a[index]);
Вот эта строчка у меня, как у питониста вызывает вопросы. Функция Append изменяет sb, но тогда по идее она не должна возвращает ничего, как к ней еще раз применяется Append?
Или это нормальная практика в C#, что функция изменяющая объект еще и ссылку какую-то на него возвращает?

Я не C# программист, но ошибку нашел, она вот тут
++index
Sign up to leave a comment.