Мы используем куки, чтобы пользоваться сайтом
было удобно.
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
С целью популяризации анализатора кода PVS-Studio, который научился проверять помимо C++, ещё и C# проекты, мы решили проверить исходный код WPF примеров, предлагаемых компанией Microsoft.
С выходом Windows Vista, была представлена новая система для построения красивых клиентских приложений - Windows Presentation Foundation (WPF). Данная графическая подсистема включена в состав .NET Framework начиная с версии 3.0. Она использует язык разметки XAML и пришла на смену устаревшему WinForms. На мой взгляд, основным недостатком WinForms было то, что он осуществлял всю прорисовку на центральном процессоре. WPF поступала более логически и отдавала прорисовку своих компонентов DirectX. Сейчас WPF практически вытеснило WinForms и позволяет делать универсальные интерфейсы сразу для трех платформ (PC, XBOXOne, Winphone).
Для анализа исходных кодов WPF примеров от Microsoft (исходники примеров) был использован статический анализатор кода PVS-Studio версии 6.05.
Интересной особенностью рассматриваемого Solution является то, что наряду с C# проектами там содержатся проекты, написанные на С++. Об этой особенности я узнал только из списка ошибок, найденных PVS-Studio. Плагин PVS-Studio для Visual Studio без каких-либо дополнительных манипуляций от пользователя проанализировал и вывел в сообщения, относящиеся как к C#, так и к С++ коду.
Рисунок 1. В окне PVS-Studio одновременно отображаются предупреждения, относящиеся как к C#, так и C++ коду проверенного проекта (нажмите на рисунок для увеличения).
1. Ошибки при составлении условий инструкции if
Ошибки при сравнении чего-либо с чем-либо - весьма распространённая проблема у программистов. Давайте пройдемся по ним.
В данном исходном коде присутствует два абсолютно одинаковых условия:
public int Compare(GlyphRun a, GlyphRun b)
{
....
if (aPoint.Y > bPoint.Y) // <=
{
return -1;
}
else if (aPoint.Y > bPoint.Y) // <=
{
result = 1;
}
else if (aPoint.X < bPoint.X)
{
result = -1;
}
else if (aPoint.X > bPoint.X)
{
result = 1;
}
....
}V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 418, 422. txtserializerwriter.cs 418
Что именно они хотели сравнить во втором случае - не совсем ясно, но явно не то, что написали.
Мы поголовно пишем проверки на null в условия, чем оберегаем программу от внештатных ситуаций. Можно сказать, что большая часть условий if состоит именно в проверках на null каких- либо полей или переменных. Вместе с тем, иногда проверки бывают не только излишни, но и могут таить в себе логическую ошибку:
public static string FindNumeric(string content)
{
string[] values = content.Split(' ');
if (values != null)
{
return values[0];
}
return "none";
}V3022 Expression 'values != null' is always true. Util.cs 287
Можно предположить, что автор хотел проверить, что values содержит больше 0 элементов, но я так и не смог придумать ситуации, когда Split вернет пустой массив. В любом случае, как ни крути, проверка на null здесь была совершенно лишняя.
Как я говорил, проект содержит код из С++ и C# диагностик. У меня сложилось впечатление, что к следующему коду приложил руку именно C++ программист.
private void LoadNotes()
{
var fs = new FileStream("NotesFile", FileMode.Open);
if (fs != null)
{
....
}V3022 Expression 'fs != null' is always true. MainWindow.cs 66
На самом деле, даже в C++ коде такой вариант ошибочен, а в C# так вообще "странно" смотрится. Почему так неверно писать в C++ коде, описано в статье "Проверяем исходный код 7-Zip с помощью PVS-Studio", ну, а мы продолжим заниматься разбором C# кода.
Далеко от подобных ситуаций нам уйти не удалось. В Solution нашлось две практически одинаковых функции (спасибо "copy-paste") с одной и той же ошибкой:
private void SerializeObjectTree(object objectTree)
{
TextWriter writer = new StreamWriter(_stream);
try
{
string fileContent =
XamlRtfConverter.ConvertXamlToRtf(
XamlWriter.Save(objectTree));
writer.Write(fileContent);
}
finally
{
if (writer != null)
writer.Close();
}
}V3022 Expression 'writer != null' is always true. htmlserializerwriter.cs 324
Ну не будет writer нулевой ссылкой...
Кидать ошибку в исключительных ситуациях - не самое плохое решение. Главное - не ошибиться в условии, когда нужно выкидывать исключение, потому что можно создать у пользователя вашего приложения весьма неприятное впечатление, когда программа падает на ровном месте.
protected Size SizeParser(string o)
{
....
if (sizeString.Length == 0 || sizeString.Length != 2)
throw new ApplicationException("Error: a size should
contain two double that seperated by a space
or ',' or ';'");
....
}V3023 Consider inspecting the 'sizeString.Length == 0 || sizeString.Length != 2' expression. The expression is excessive or contains a misprint. MainWindow.cs 140
Судя по тексту ошибки, в данном случае, проверка на 0 излишняя, достаточно было проверить sizeString.Length на неравенство 2.
В длинных телах инструкции if очень сложно обнаружить бессмысленные проверки глазами.
private static void WriteElement(....)
{
if (htmlWriter == null)
{
....
}
else
{
if (htmlWriter != null && htmlElementName != null)
{
....
....
}V3063 A part of conditional expression is always true: htmlWriter != null. HtmlFromXamlConverter.cs 491
Для анализатора это не проблема. Кстати, благодаря любимому нашему копированию-вставке, ошибка была найдена сразу в двух проектах: HtmlToXamlDemo и DocumentSerialization.
Конечно, бессмысленные проверки встречаются не только в длинных функциях, но и в пределах двух-трех строчек.
private void OnFlipPicTimeline(object sender, EventArgs e)
{
var clock = (Clock) sender;
if (clock.CurrentState == ClockState.Active) // Begun case
{
return;
}
if (clock.CurrentState != ClockState.Active) // Ended case
{
....
}
}V3022 Expression 'clock.CurrentState != ClockState.Active' is always true. MainWindow.cs 103
В принципе, ничего страшного, но, когда после идет инструкция if вложенная еще в один if и еще и еще... Хочется избавиться хотя бы от бессмысленных проверок для лучшего восприятия кода, который, на самом деле, читается гораздо чаше чем пишется.
Давайте немного передохнём. И посмотрим на вот такую необычную функцию, которая мне встретилась. Тело функции приведено полностью:
private void UpdateSavings()
{
Savings = TotalIncome - (Rent + Misc + Food);
if (Savings < 0)
{
}
else if (Savings >= 0)
{
}
}V3022 Expression 'Savings >= 0' is always true. NetIncome.cs 98
Также было найдено много (более 60) сравнений вещественных чисел (double) с конкретным 0.
if (leftConst != null && leftConst.Value == 0)
{
// 0 + y; return y;
return newRight;
}К примеру:
Статьи не хватит все строчки привести. Данное предупреждение у нас находится на 3-м уровне, т.к. её актуальность сильно зависит от специфики программы. В случае, если над числами производится математические вычисления (манипуляциях со значением), то не гарантируется, что мы получим конкретное число: -1, 0, 1. Отклонение в 0,00000000001уже приведет к неверному результату сравнения. Однако, если логика программы предполагает запись в вещественные числа (double) дискретных значений, то подобные проверки не являются ошибкой.
2. Ошибки в инициализации и при присвоении переменных
Функция - отличная штука, которая не только позволяет убрать повторяющиеся код, но и упростить понимание участка кода, где эта функция используется. Особенно важно, чтобы функция выполняла именно ту задачу, которая описана в её имени и сигнатуре вызова. Но так бывает не всегда. Возьмем, например, следующий участок кода. Функция приведена полностью для лучшего понимания ситуации.
public bool OpenDocument(string fileName)
{
Microsoft.Win32.OpenFileDialog dialog;
// If there is a document currently open, close it.
if (this.Document != null) CloseFile();
dialog = new Microsoft.Win32.OpenFileDialog();
dialog.CheckFileExists = true;
dialog.InitialDirectory = GetContentFolder();
dialog.Filter = this.OpenFileFilter;
bool result = (bool)dialog.ShowDialog(null);
if (result == false) return false;
fileName = dialog.FileName; // <=
return OpenFile(fileName);
}V3061 Parameter 'fileName' is always rewritten in method body before being used. ThumbViewer.xaml.cs 192
Имя файла, которое предполагается открыть, перетирается прямо перед его первым использованием fileName = dialog.FileName. Да, в данном случае, откроется диалоговое окно и будет выбран файл пользователя, но зачем тогда нужен параметр, который по факту не используется?
Нехватка времени и копирование-вставка иногда порождают весьма странные конструкции:
public MailSettingsDialog()
{
....
_timerClock = _timerClock = new DispatcherTimer();
....
}V3005 The '_timerClock' variable is assigned to itself. MailSettingsDialog.cs 56
Казалось бы, не самая страшная опечатка, но она наталкивает на мысль, а туда ли мы "пишем во второй раз". Ну, например, как здесь:
private void DumpAllClipboardContentsInternal()
{
....
if (dataObject == null)
{
clipboardInfo.Text =
clipboardInfo.Text =
"Can't access clipboard now!
\n\nPlease click Dump All Clipboard
Contents button again.";
}
else
{
....
}V3005 The 'clipboardInfo.Text' variable is assigned to itself. MainWindow.cs 204
Вообще, код изобилует странными присваиваниями:
private void DoParse(string commandLine)
{
....
strLeft = strRight = string.Empty;
strLeft = strs[0];
strRight = strs[1];
....
}V3008 The 'strLeft' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 55, 54. CommandLine.cs 55
V3008 The 'strRight' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 56, 54. CommandLine.cs 56
strLeft и strRight - это просто локальные переменные типа string.
Ну или вот, дальнейший пример кода, более некорректный. В нем зачем-то так много чего-то считали, а после еще раз пересчитали и записали в туже переменную.
private object InvokMethod(....)
{
arg = commandLine.Substring(
commandLine.IndexOf("(", StringComparison.Ordinal) + 1,
commandLine.IndexOf(")",
StringComparison.Ordinal) -
(commandLine.IndexOf("(",
StringComparison.Ordinal) + 1));
arg = commandLine.Substring(
commandLine.IndexOf("(",
StringComparison.Ordinal) + 1);
}V3008 The 'arg' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 176, 173. CommandLine.cs 176
И еще много-много таких или подобных примеров бессмысленного первичного присвоения:
private void DrawFormattedText(DpiScale dpiInfo)
{
....
Geometry geometry = new PathGeometry();
geometry = formattedText.BuildGeometry(
new System.Windows.Point(0, 0));
....
}Писать каждый пример смысла нет, тем более нас ожидает ещё много важных и серьезных ошибок.
3. Пара разнородных ошибок
Пробрасывая исключения, важно сохранить стек вызова, чтобы потом по логам понять - "а что у пользователя в принципе-то упало". Далеко не все знают, как правильно это делать.
public static object InvokePropertyOrMethod(....)
{
try
{
....
}
catch (MissingMethodException e)
{
....
throw e;
}
catch (AmbiguousMatchException e)
{
throw e;
}
return resultObject;
}V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost. ReflectionUtils.cs 797
V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost. ReflectionUtils.cs 806
Согласно стандарту, если передавать exception выше по стеку вызова функций способом throw e;, мы потеряем стек вызова, который был до перехвата исключения в блоке catch. Для сохранения всего стека вызова и его дальнейшего продолжения, нужно просто написать одно слово throw в блоке catch и всё.
Иногда проверки излишни, а иногда их просто не хватает, как например в следующем коде:
private static void ParseCssFontFamily(....)
{
....
if (fontFamilyList == null && fontFamily.Length > 0)
{
if (fontFamily[0] == '"' || fontFamily[0] == '\'')
{
// Unquote the font family name
fontFamily =
fontFamily.Substring(1, fontFamily.Length - 2);
....
}V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. HtmlCSSParser.cs 645
Здесь нет проверки, что fontFamily.Length больше 1, и в следствии этого вычитания из fontFamily.Length числа 2, мы можем получить значение меньше 0. А данная функция в таких случаях выкидывает исключение ArgumentOutOfRangeException.
Безопасней было бы написать проверку:
if (fontFamilyList == null && fontFamily.Length > 1)4. WPF ошибка
DependencyProperty - одна из самых замечательных возможностей WPF. Создавать свойства, которые прямо из коробки могут оповестить разработчика о своём изменении - невероятно удобно. Но главное - не перепутать сигнатуру для их описания, особенно важно об этом помнить, показывая примеры, потому что именно на них люди и ориентируются.
public double Radius
{
get { return (double) GetValue(RadiusProperty); }
set { SetValue(RadiusProperty, value); }
}
public static readonly DependencyProperty
RadiusProperty = DependencyProperty.Register(
"RadiusBase",
typeof (double),
typeof (FireworkEffect),
new FrameworkPropertyMetadata(15.0));V3045 WPF: the names of the registered property 'RadiusBase', and of the property 'Radius', do not correspond with each other. FireworkEffect.cs 196
В данном конкретном случае, имя, которое зарегистрировано для свойства зависимости, не совпадает с именем свойства-обертки для доступа с DependencyProperty из кода. Такой вариант приводит к большим проблемам при работе из XAML разметки. WPF позволяет из XAML обратиться к простому свойству Radius и прочитать от туда значение, но вот изменения данного свойства подхватываться из XAML не будут.
На самом деле, в PVS-Studio имеется целый ряд диагностик для обнаружения ошибок в сигнатуре создания DependencyProperty [3044, 3045, 3046, 3047, 3048, 3049]. Большинство ошибок такого рода приводят к падению программы, как только начинается использование классас данными свойствами зависимости. Поэтому данные диагностики предназначены именно для того, чтобы избавить вас от поиска и анализа длинных портянок сигнатур, особенно после копирования. Для этого, естественно, нужна регулярная проверка кода, а не только анализ финальной версии программы.
Рассмотрим еще одно интересное срабатывание. В данном случае сработала новая диагностика V3095. Данная диагностика показывает места, где мы сначала обращаемся к переменной, а потом проверяем её на равенство null.
private static XmlElement AddOrphanListItems(....)
{
Debug.Assert(htmlLiElement.LocalName.ToLower() == "li");
....
XmlNode htmlChildNode = htmlLiElement;
var htmlChildNodeName = htmlChildNode == null
? null
: htmlChildNode.LocalName.ToLower();
....
}V3095 The 'htmlLiElement' object was used before it was verified against null. Check lines: 916, 936. HtmlToXamlConverter.cs 916
В данном случае, в условии тернарного оператора мы проверяем, что переменная htmlChildNode может быть null. При этом переменная htmlChildNode представляет собой не более чем ссылку на переменную htmlLiElement. А вот именно к переменной htmlLiElement мы обращались без проверки на null. В итоге, мы либо имеет код, который никогда не выполнится, либо вообще получим исключение NullReferenceException в строке htmlLiElement.LocalName.ToLower().
Кроме описанных ошибок, большое внимание к себе привлекает диагностика под номером V3072, которая предназначена для выявления наличия полей с типом, который реализует интерфейс IDisposable, но сам класс, где поля объявлены, не имеет подобной реализации.
internal class Email
{
private readonly SmtpClient _client;
....
}V3072 The 'Email' class containing IDisposable members does not itself implement IDisposable. Inspect: _client. Email.cs 15
С IDisposable всегда было туго. От критических ошибок, в следствии неправильного его использования, кончено, часто спасает Finalize, ну, по крайней мере, в стандартных классах. Программисты нередко забивают, забывают, упускают или просто не обращают внимания на поля с типом, реализующим данный интерфейс. Оправдать подобный код или признать в нем наличие ошибки сложно для стороннего взгляда, но есть некоторые паттерны, на которые стоит обратить внимание. В данном Solution, подобных срабатываний набралось тоже не мало.
1. Ошибки при составлении условий инструкции if
Для меня, конечно, было откровением, что я найду в данном Solution С++ проекты, но тем не менее, ошибки есть ошибки и давайте на них посмотрим.
Как и в C#, начнем с различных сравнений и сразу рассмотрим ту самую С++ ошибку о которой я говорил чуть выше в C# блоке.
STDMETHOD(CreateInstance)(....)
{
....
T *obj = new T();
if (NULL != obj)
{
....
}V668 There is no sense in testing the 'obj' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. classfactory.h 76
Если оператор new не смог выделить память, то, согласно стандарту языка C++, генерируется исключение std::bad_alloc(). Таким образом, проверять на равенство нулю не имеет смысла, т.к. указатель obj никогда не будет равен NULL. Если выделить память невозможно, то возникает исключение, которое можно обрабатывать на более высоком уровне, а проверку на равенство NULL можно просто удалить. Если исключения в приложении нежелательны, то можно использовать оператор new, не генерирующий исключений (T *obj = new (std::nothrow) T()): в этом случае можно проверять возвращаемое значение на ноль. В Solution встретилось еще 4 подобных проверки:
Лишние условия актуальны в обоих языках программирования:
if (bitmapLock && bitmap)
{
if(bitmapLock)
{
bitmapLock->Release();
bitmapLock = NULL;
}
}V571 Recurring check. The 'bitmapLock' condition was already verified in line 104. aitdecoder.cpp 106
Некоторые программисты C# не знают, что следующие две операции над Nullable типом равнозначные:
И делают подобные проверки:
if (_isInDesignMode != null && _isInDesignMode.HasValue)В C++ любят бессмысленно проверять указатель на null, перед тем как освободить память, выделенную по адресу, на который он указывает.
static HRESULT OutputColorContext(....)
{
....
if (pixels)
delete[] pixels;
....
}V809 Verifying that a pointer value is not NULL is not required. The 'if (pixels)' check can be removed. aitencoder.cpp 189
static HRESULT OutputBitmapPalette(....)
{
....
if (colors)
delete[] colors;
....
}V809 Verifying that a pointer value is not NULL is not required. The 'if (colors)' check can be removed. aitencoder.cpp 241
static HRESULT OutputColorContext(....)
{
if (bytes)
delete[] bytes;
}V809 Verifying that a pointer value is not NULL is not required. The 'if (bytes)' check can be removed. aitencoder.cpp 292
2. Логическая ошибка
Следующий код представляет весьма интересную ситуацию логического сравнения, хотя на первый взгляд так и не скажешь:
STDMETHODIMP AitDecoder::QueryCapability(....)
{
....
// If this is our format, we can do everything
if (strcmp(bh.Name, "AIT") == 0)
{
*pCapability =
WICBitmapDecoderCapabilityCanDecodeAllImages ||
WICBitmapDecoderCapabilityCanDecodeThumbnail ||
WICBitmapDecoderCapabilityCanEnumerateMetadata ||
WICBitmapDecoderCapabilitySameEncoder;
}
....
}V560 A part of conditional expression is always true. aitdecoder.cpp 634
Диагностика посчитала, что часть выражения всегда истина и она права, т.к. слова WICBitmapDecoderCapabilityCanDecodeXXX представляют собой просто значения enum с именем WICBitmapDecoderCapabilities:
enum WICBitmapDecoderCapabilities
{
WICBitmapDecoderCapabilitySameEncoder = 0x1,
WICBitmapDecoderCapabilityCanDecodeAllImages = 0x2,
WICBitmapDecoderCapabilityCanDecodeSomeImages = 0x4,
WICBitmapDecoderCapabilityCanEnumerateMetadata = 0x8,
WICBitmapDecoderCapabilityCanDecodeThumbnail = 0x10,
WICBITMAPDECODERCAPABILITIES_FORCE_DWORD = 0x7fffffff
};Как следствие, вероятно, что кто-то просто перепутал символы и вместо побитового ИЛИ "|" написали логическое ИЛИ "||". В отличии от C# компилятора, С++ это легко проглотил.
3. Ошибки в инициализации и при присвоении переменных
Кончено, после рефакторинга часто остаются два раза подряд проинициализированные переменные.
STDMETHODIMP BaseFrameEncode::WritePixels(....)
{
result = S_OK;
....
result = factory->CreateBitmapFromMemory(....);
}V519 The 'result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 269, 279. baseencoder.cpp 279
Когда переменные инициализируется через несколько строк кода, то легко можно понять человека, почему он промахнулся, но иногда подобные две строчки идут подряд:
STDMETHODIMP AitFrameEncode::Commit()
{
HRESULT result = E_UNEXPECTED;
result = BaseFrameEncode::Commit();
....
}V519 The 'result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 320, 321. aitencoder.cpp 321
Бытует мнение, что C# код менее подвержен ошибкам, чем С++; в определенных случаях это действительно так. Однако, интересный факт состоит в том, что основная масса ошибок находится не в специфических конструкциях, а в простых выражениях. Например, в условиях инструкции if. Статический анализатор кода для PVS-Studio C, C++ и C# позволит вам контролировать качество вашего кода и всеми силами будет уберегать вас от фатальных ошибок, которые могут дойти до вашего пользователя.
0
0
0
0