Comments 9
Автор метода findNextMenuItem хочет избавиться от -1, возвращаемой методом indexOf, в случае, если список menuItemWidgets не содержит currentItem
Не, тут дело похоже в другом.
Судя по названию метода «findNextMenuItem» и названию переменной «previousMenuItemFlatIndex» предположу что дело было так:
- Был метод «findPreviousMenuItem» где было что-то вроде:
int previousMenuItemFlatIndex = menuItemFlatIndex - 1; if (previousMenuItemFlatIndex >= 0) { return menuItemWidgets.get(previousMenuItemFlatIndex); }
- Кто-то взял и скопировал этот метод, назвав его «findNextMenuItem»
- Заменил " — 1" на " + 1" (чтобы взять следующий элемент вместо предыдущего), а все остальное — названия переменных и проверки — заменить забыл.
Картинки со звуком!
Сам хит-парад ошибок, увы, скучный. Копипаст и невнимательность.
Хотелось ошибок хотя бы такого уровня.
Кстати, PVS-Studio ошибку из статьи по ссылке найдёт?
Ну вот как раз подобный кейс со скоррелированным состоянием в том же методе, который разбирается на пятом месте (смотри мой комментарий ниже). Почему-то в хит-парад попал не он, а ложное срабатывание =)
if (text == null || text.length() < 2) {
return false;
}
if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {// <=
return false;
}
Я ж вам про это говорил вроде. Это вообще не ошибка, просто перестраховка. Вы смотрите хоть немного логику метода. Да, если на входе строка "0"
, она отсекается двумя разными способами, в обоих случаях return false;
произойдёт, поэтому без разницы, в какую ветку мы зайдём. Может сбить с толку, если не вчитаться в код, но ошибки здесь нет. Сама Идея этот код подсвечивает ещё с 2017-го года (и не заводите волынку, что раз мы его не исправили, то нищитово).
final int roundCarryMask = (1 << (bitShiftsInWord - 1)); // <=
Это тоже вообще не ошибка. Я потому не делаю подобные unsound-варнинги, потому что в них сбалансировать false-positive/false-negative очень сложно. Здесь откровенно мусорный варнинг, который только отвлечёт программистов от реальных проблем. Видите выше noRestore = bitShiftsInWord == 0
? А ниже посмотрите на использования noRestore
. Вы сразу увидите, что когда bitShiftsInWord == 0
, результат битового сдвига (переменная roundCarryMask
) не используется вообще. Поэтому абсолютно наплевать, какое там значение.
Почему-то вы, кстати, молчите про другую вещь в том же методе:
if (wordShifts == 0 && bitShiftsInWord == 0) {
return;
}
...
final boolean noRestore = bitShiftsInWord == 0;
...
switch (wordShifts) {
case 0:
// noRestore is always false
roundCarry = (noRestore ? 0 : (this.v[0] & roundCarryMask)) != 0;
...
Здесь очевидно noRestore
всегда ложно, потому что случай wordShifts == 0 && bitShiftsInWord == 0
был обработан выше. Идея радостно подсвечивает эти ветки и предлагает автоматически упростить код в один клик. PVS-Studio не может так? ;-)
В целом:
- Идея репортит 10, 9 (первый кейс), 8, 7, 6, 4.
- 5 репортить и не стоит, смотрите выше
- 9.2 репортить было бы хорошо, но это тоже unsound warning (а вдруг снаружи метода всегда проверяется, что
x != index_64.length
?). У меня была черновая реализация, но возникают помимо хороших варнингов реально очень мутные false-positive, где голову сломаешь перед тем как докажешь, что анализатор неправ. Я поэтому убрал этот код. Возможно, стоит вернуться. - 3 должен репортиться инспекцией Integer multiplication or shift implicitly cast to long, но почему-то не срабатывает. Проверю, починю, спасибо!
- 2 — это интересная штука, у нас прямого аналога нет. Кое-какие циклы инициализации репортятся косвенно, но прямо такой нету.
- 1 — варнинг крутой, но как я понял инспекция у вас эвристическая. Возникает вопрос, сколько мусора она репортит. Вообще преаллоцированные массивы в
toArray()
в наши дни — антипаттерн. У нас на это есть инспекция, которая подсвечивает верхний по дефолту, но как раз молчит в нижнем, потому что вдруг пользователь реально хотел массив другой длины (чтобы были null'ы в хвосте).
Топ 10 ошибок в проектах Java за 2019 год