Geekly Articles each Day
📜 ⬆️ ⬇️

Checking the source code of WPF Samples from Microsoft

In order to popularize the PVS-Studio code analyzer, which learned to test, in addition to C ++, also C # projects, we decided to check the source code of WPF examples offered by Microsoft.



With the release of Windows Vista, a new system for building beautiful client applications was introduced - the Windows Presentation Foundation (WPF). This graphics subsystem is included in the .NET Framework starting with version 3.0. It uses the XAML markup language and has replaced the outdated WinForms. In my opinion, the main disadvantage of WinForms was that it carried out all the drawing on the central processor. WPF acted more logically and rendered its DirectX components. Now WPF has practically supplanted WinForms and allows you to make universal interfaces for three platforms at once (PC, XBOXOne, Winphone).

For analyzing the source code of WPF examples from Microsoft ( source ), the PVS-Studio static code analyzer version 6.05 was used.
')
An interesting feature of this Solution is that, along with C # projects, it contains projects written in C ++. I learned about this feature only from the list of errors found by PVS-Studio. The PVS-Studio plugin for Visual Studio without any additional manipulations from the user analyzed and outputted in messages related to both C # and C ++ code.

Figure 1. In the PVS-Studio window, warnings are simultaneously displayed.


Figure 1. In the PVS-Studio window, at the same time, warnings related to both C # and C ++ code of the checked project are displayed (click on the image to enlarge).

C # Errors


1. Errors in the preparation of the conditions of the if statement

Errors when comparing something with something is a very common problem for programmers. Let's go over them.

In this source code there are two absolutely identical conditions:
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 possibility of logical error presence. Check lines: 418, 422. txtserializerwriter.cs 418

What exactly they wanted to compare in the second case is not entirely clear, but obviously not what they wrote.

We politely write checks to null in the conditions, than we protect the program from emergency situations. It can be said that most of the if conditions consist precisely in checking for null of any fields or variables. At the same time, sometimes checks are not only superfluous, but can also conceal a logical error:
 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

We can assume that the author wanted to check that the values contain more than 0 elements, but I could not think of a situation when Split returns an empty array. In any case, whatever one may say, the check for null here was completely superfluous.

As I said, the project contains code from C ++ and C # diagnostics. I got the impression that the C ++ programmer had a hand in the following code.
 private void LoadNotes() { var fs = new FileStream("NotesFile", FileMode.Open); if (fs != null) { .... }

V3022 Expression 'fs! = Null' is always true. MainWindow.cs 66

In fact, even in C ++ code, this option is erroneous, but in C # it looks “strange” in general. Why is it wrong to write in C ++ code, described in the article " Checking 7-Zip source code with PVS-Studio ", well, we will continue to analyze C # code.

Far from such situations, we could not escape. The Solution found two practically identical functions (thanks to “copy-paste”) with the same error:
 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

Well, there will be no zero link writer ...

Throwing a mistake in exceptional situations is not a bad decision. The main thing is not to make a mistake in the condition when you need to throw an exception, because you can create a very unpleasant impression for the user of your application when the program falls out of the blue.
 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 misprint. MainWindow.cs 140

Judging by the text of the error, in this case, checking for 0 is excessive, it was enough to check sizeString.Length for inequality 2.

In long bodies, if statements are very difficult to detect meaningless eye checks.
 private static void WriteElement(....) { if (htmlWriter == null) { .... } else { if (htmlWriter != null && htmlElementName != null) { .... .... }

V3063 A part of the conditional expression is always true: htmlWriter! = Null. HtmlFromXamlConverter.cs 491

For the analyzer this is not a problem. By the way, thanks to our favorite copy-paste, the error was found in two projects at once: HtmlToXamlDemo and DocumentSerialization .

Of course, meaningless checks are found not only in long functions, but also within two or three lines.
 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

In principle, nothing terrible, but when after there is an if statement nested in one if and more and more ... I want to get rid of at least senseless checks for better perception of the code, which, in fact, is read much more often than it is written.

Let's take a little rest. And look at this unusual function that I met. The body of the function is given in full:
 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

It was also found many (more than 60) comparisons of real numbers ( double ) with a specific 0.
 if (leftConst != null && leftConst.Value == 0) { // 0 + y; return y; return newRight; }

For example:
Articles are not enough to bring all the lines. This warning is on the 3rd level, since its relevance is highly dependent on the specific program. If mathematical calculations are performed on numbers (manipulations with a value), it is not guaranteed that we will get a specific number: -1, 0, 1. A deviation of 0.00000000001 will already lead to an incorrect comparison result. However, if the program's logic involves writing discrete values ​​to real numbers ( double ), then such checks are not an error.

2. Errors in initialization and in the assignment of variables

The function is a great thing, which not only allows you to remove duplicate code, but also simplify the understanding of the section of code where this function is used. It is especially important that the function performs exactly the task that is described in its name and the call signature. But this is not always the case. Take, for example, the following piece of code. The function is given fully for a better understanding of the situation.
 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

The name of the file to be opened is ground right before its first use. FileName = dialog.FileName . Yes, in this case, a dialog box opens and a user file is selected, but then why do you need a parameter that is not actually used?

The lack of time and copy-paste sometimes produce very strange constructions:
 public MailSettingsDialog() { .... _timerClock = _timerClock = new DispatcherTimer(); .... }

V3005 The '_timerClock' variable is assigned to itself. MailSettingsDialog.cs 56

It would seem, not the most terrible typo, but it suggests the idea whether we are “writing a second time”. Well, for example, like this:
 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

In general, the code is replete with strange assignments:
 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 and strRight are just local variables of type string .

Well, or here, a further code sample, more incorrect. For some reason, so much something was counted in it, and after that it was again counted and recorded in the same variable.
 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

And there are many, many such or similar examples of meaningless primary appropriation:
 private void DrawFormattedText(DpiScale dpiInfo) { .... Geometry geometry = new PathGeometry(); geometry = formattedText.BuildGeometry( new System.Windows.Point(0, 0)); .... }

It makes no sense to write each example, especially since there are still many more important and serious mistakes.

3. A pair of mixed errors

Throwing exceptions, it is important to save the call stack, so that later you can see from the logs - “but what did the user basically drop?” Not everyone knows how to do it correctly.
 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

According to the standard, if you pass exception higher in the function call stack by the method throw e; , we will lose the call stack that was before the exception was caught in the catch block . To save the entire call stack and continue it, you just need to write one word throw in the catch block and that's it.

Sometimes checks are unnecessary, and sometimes they are simply not enough, as for example in the following code:
 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

There is no check that fontFamily.Length is greater than 1, and as a result of this subtraction from the fontFamily.Length number 2, we can get a value less than 0. And this function throws out an ArgumentOutOfRangeException in such cases.

It would be safer to write a check:
 if (fontFamilyList == null && fontFamily.Length > 1)

4. WPF Error

DependencyProperty is one of the great features of WPF. Creating properties out of the box can notify the developer of their change is incredibly convenient. But the main thing is not to confuse the signature for their description; it is especially important to keep this in mind, showing examples, because it is them who are oriented on.
 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: 'RadiusBase', and the property of 'Radius', do not correspond with each other. FireworkEffect.cs 196

In this particular case, the name that is registered for the dependency property does not match the name of the wrapper property to access with the DependencyProperty from the code. This option leads to big problems when working from XAML markup. WPF allows XAML to access the simple Radius property and read the value from there, but changes to this property will not be picked up from the XAML.

In fact, in PVS-Studio there are a number of diagnostics for detecting errors in the signature of the creation of the DependencyProperty [ 3044 , 3045 , 3046 , 3047 , 3048 , 3049 ]. Most errors of this kind lead to the crash of the program as soon as the use of the class with these dependency properties begins. Therefore, these diagnostics are intended precisely to save you from searching and analyzing long signature signatures, especially after copying. For this, of course, you need regular code checking, and not just analysis of the final version of the program.

Consider another interesting trigger. In this case, the new V3095 diagnostics worked. This diagnostic shows the places where we first refer to the variable, and then check it for equality 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 verified against it. Check lines: 916, 936. HtmlToXamlConverter.cs 916

In this case, in the condition of the ternary operator, we check that the htmlChildNode variable can be null . In this case, the htmlChildNode variable is nothing more than a reference to the htmlLiElement variable. But it was the htmlLiElement variable that we accessed without checking for null . As a result, we either have code that will never execute, or we generally get a NullReferenceException exception in the htmlLiElement.LocalName.ToLower () string.

In addition to the errors described, a lot of attention is attracted by the diagnostics number V3072 , which is designed to detect the presence of fields with a type that implements the IDisposable interface, but the class itself, where the fields are declared, does not have a similar implementation.
 internal class Email { private readonly SmtpClient _client; .... }

V3072 The 'Email' class containing IDisposable members doesn’t itself implement IDisposable. Inspect: _client. Email.cs 15

With IDisposable it has always been tight. From critical errors, due to improper use, is over, often saves Finalize , well, at least in standard classes. Programmers often slaughter, forget, miss, or simply do not pay attention to the fields with the type that implements this interface. To justify such a code or to recognize in it the presence of an error is difficult for an outsider look, but there are some patterns that you should pay attention to. In this Solution, there were not a few such positives.

C ++ Errors


1. Errors in the preparation of the conditions of the if statement

For me, of course, it was a revelation that I would find projects in this Solution C ++ project, but nevertheless, there are errors in mistakes and let's look at them.

As in C #, let's start with various comparisons and immediately consider the very C ++ error I mentioned just above in the C # block.
 STDMETHOD(CreateInstance)(....) { .... T *obj = new T(); if (NULL != obj) { .... }

V668 has been allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. classfactory.h 76

If the new operator could not allocate memory, then, according to the C ++ standard of the language, an exception is thrown std :: bad_alloc (). Thus, it does not make sense to check for equality to zero, since pointer obj will never be NULL . If it is impossible to allocate memory, an exception is raised that can be processed at a higher level, and the test for NULL equality can simply be removed. If exceptions in the application are undesirable, you can use the new operator, which does not generate exceptions ( T * obj = new (std :: nothrow) T () ): in this case, you can check the return value to zero. In Solution, there were 4 more such checks:
The extra conditions are relevant in both programming languages:
 if (bitmapLock && bitmap) { if(bitmapLock) { bitmapLock->Release(); bitmapLock = NULL; } }

V571 Recurring check. The 'bitmapLock' condition was already verified in line 104. aitdecoder.cpp 106

Some C # programmers do not know that the following two operations on the Nullable type are equivalent:
And they make similar checks:
 if (_isInDesignMode != null && _isInDesignMode.HasValue)

In C ++, it makes no sense to check a null pointer before freeing the memory allocated to the address it points to.
 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 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 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. Logical error

The following code represents a very interesting situation of logical comparison, although at first glance one cannot say:
 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 the conditional expression is always true. aitdecoder.cpp 634

Diagnostics considered that part of the expression is always true and it is right, because the words WICBitmapDecoderCapabilityCanDecodeXXX are simply enum values ​​with the name WICBitmapDecoderCapabilities :
 enum WICBitmapDecoderCapabilities { WICBitmapDecoderCapabilitySameEncoder = 0x1, WICBitmapDecoderCapabilityCanDecodeAllImages = 0x2, WICBitmapDecoderCapabilityCanDecodeSomeImages = 0x4, WICBitmapDecoderCapabilityCanEnumerateMetadata = 0x8, WICBitmapDecoderCapabilityCanDecodeThumbnail = 0x10, WICBITMAPDECODERCAPABILITIES_FORCE_DWORD = 0x7fffffff };

As a result, it is likely that someone just confused the characters and instead of the bitwise OR "|" wrote a logical OR "||". Unlike the C # compiler, C ++ is easily swallowed.

3. Errors in initialization and assignment of variables

Of course, after refactoring, the initialized variables often remain twice in a row.
 STDMETHODIMP BaseFrameEncode::WritePixels(....) { result = S_OK; .... result = factory->CreateBitmapFromMemory(....); }

V519 The 'result' variable is assigned twice successively. Perhaps this is a mistake. Check lines: 269, 279. baseencoder.cpp 279

When variables are initialized in a few lines of code, you can easily understand the person why he missed, but sometimes these two lines go in a row:
 STDMETHODIMP AitFrameEncode::Commit() { HRESULT result = E_UNEXPECTED; result = BaseFrameEncode::Commit(); .... }

V519 The 'result' variable is assigned twice successively. Perhaps this is a mistake. Check lines: 320, 321. aitencoder.cpp 321

Conclusion


There is an opinion that C # code is less error prone than C ++; in certain cases this is true. However, an interesting fact is that the majority of errors are not in specific constructions, but in simple expressions. For example, in terms of if statements . The static code analyzer for PVS-Studio C, C ++ and C # will allow you to control the quality of your code and will by all means save you from fatal errors that may reach your user.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Alferov Vitalii. Source code of WPF samples by Microsoft was checked .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/304206/

More articles:


All Articles