Code analysis fixes

jeanbern · jeanbern · commit 6e5391272c52 · 2019-11-08T17:00:40.000-07:00
diff --git a/.editorconfig b/.editorconfig
@@ -0,0 +1,4 @@
+﻿[*.cs]
+
+# IDE0004: Remove Unnecessary Cast
+dotnet_diagnostic.IDE0004.severity = warning
diff --git a/AllProjects.sln b/AllProjects.sln
@@ -13,6 +13,7 @@ EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{E866D3C8-0885-4C79-BA12-9DE39C922925}"
 	ProjectSection(SolutionItems) = preProject
 		.codecov.yml = .codecov.yml
+		.editorconfig = .editorconfig
 		appveyor.yml = appveyor.yml
 		LICENSE = LICENSE
 		README.md = README.md
diff --git a/Portent.Benchmark/DawgBenchmark.cs b/Portent.Benchmark/DawgBenchmark.cs
@@ -84,8 +84,11 @@ public static Dawg CreateDictionary(string corpusPath, string savePath)
                 }
             }
 
-            var compressedGraph = builder.AsCompressedSparseRows();
-            compressedGraph.Save(savePath);
+            using (var compressedGraph = builder.AsCompressedSparseRows())
+            {
+                compressedGraph.Save(savePath);
+            }
+
             using var dawgStream = File.OpenRead(savePath);
             return new Dawg(dawgStream);
         }
diff --git a/Portent.Benchmark/Portent.Benchmark.csproj b/Portent.Benchmark/Portent.Benchmark.csproj
@@ -7,7 +7,7 @@
     <PlatformTarget>x64</PlatformTarget>
     <NullableContextOptions>enable</NullableContextOptions>
     <Nullable>enable</Nullable>
-    <DefineConstants>DEBUG;TRACE;BIT64</DefineConstants>
+    <DefineConstants>DEBUG;TRACE;</DefineConstants>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
 
diff --git a/Portent.Benchmark/Program.cs b/Portent.Benchmark/Program.cs
@@ -11,21 +11,18 @@ public static void Main()
         {
             if (Debugger.IsAttached)
             {
-                RunForProfiler();
+                PrintResultCounts();
             }
             else
             {
                 BenchmarkRunner.Run<DawgBenchmark>();
             }
         }
 
-        private static void RunForProfiler()
+        private static void PrintResultCounts()
         {
-            Console.WriteLine("reading");
             using var benchmark = new DawgBenchmark();
-            Console.WriteLine("setup for run");
             benchmark.SetupForRun();
-            Console.WriteLine("verify correctness");
             if (!benchmark.VerifyDawgCorrectness())
             {
                 throw new InvalidOperationException();
@@ -40,7 +37,6 @@ private static void RunForProfiler()
                 Console.WriteLine(benchmark.GetTotalResults());
             }
 
-            Console.WriteLine("Done, press {ENTER} to continue...");
             Console.ReadLine();
         }
     }
diff --git a/Portent/Allocation/Native/PrivilegeHolder.cs b/Portent/Allocation/Native/PrivilegeHolder.cs
@@ -29,7 +29,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Threading;
-
+#pragma warning disable S3453 // Classes should not have only "private" constructors
 namespace Portent
 {
     /// <summary>
diff --git a/Portent/Collections/SuggestItemCollection.cs b/Portent/Collections/SuggestItemCollection.cs
@@ -7,7 +7,7 @@
 
 namespace Portent
 {
-    internal sealed class SuggestItemCollection : IEnumerable<SuggestItem>
+    internal sealed class SuggestItemCollection : IEnumerable<SuggestItem>, IDisposable
     {
         public SuggestItemCollection(int count)
         {
@@ -123,5 +123,11 @@ public void Dispose()
                 //Empty - Required by IEnumerator<T>, but nothing to dispose.
             }
         }
+
+        public void Dispose()
+        {
+            // So useless
+            _myEnumerator.Dispose();
+        }
     }
 }
diff --git a/Portent/Dawg.cs b/Portent/Dawg.cs
@@ -1,5 +1,6 @@
 ﻿using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Diagnostics.Contracts;
 using System.IO;
 using System.Linq;
@@ -14,16 +15,11 @@
 
 // ReSharper disable BuiltInTypeReferenceStyle
 
-#if BIT64
 // Use nint when referring to pointer values, long when referring to 64 bit values.
 using nint = System.Int64;
 using nuint = System.UInt64;
 // ReSharper disable SuggestVarOrType_BuiltInTypes
 // ReSharper disable SuggestVarOrType_Elsewhere
-#else
-using nint = System.Int32;
-using nuint = System.UInt32;
-#endif
 
 namespace Portent
 {
@@ -48,9 +44,11 @@ public IEnumerable<SuggestItem> Lookup(in string word, uint maxEdits)
             _compoundResultCollection.Clear();
             var wordLength = (uint) word.Length;
 
-            // TODO: Align these allocated chunks together.
+#pragma warning disable S1135 // Track uses of "TODO" tags
+// TODO: Align these allocated chunks together.
             // + 1 for the (char)0 at the start.
             var inputLength = MemoryAlignmentHelper.GetCacheAlignedSize<char>(wordLength + 1);
+#pragma warning restore S1135 // Track uses of "TODO" tags
             var inputBytes = stackalloc byte[inputLength];
             var input = MemoryAlignmentHelper.GetCacheAlignedStart<char>(inputBytes);
             *input = (char) 0;
@@ -81,7 +79,7 @@ public IEnumerable<SuggestItem> Lookup(in string word, uint maxEdits)
             var run = stackalloc Run[1];
             run[0] = new Run(input, toCache, wordLength, maxPlusOne);
             // TODO: There's got to be a simpler way of doing this. Not a high priority though.
-            var hasRun = stackalloc bool[(int) rootLast - (int) rootFirst];
+            var hasRun = stackalloc bool[(int)(rootLast -  rootFirst)];
             for (nint i = 0; i < rootLast - rootFirst; i++)
             {
                 hasRun[i] = false;
@@ -131,6 +129,7 @@ public IEnumerable<SuggestItem> Lookup(in string word, uint maxEdits)
 
         [LocalsInit(false)]
         [MethodImpl(MethodImplOptions.AggressiveOptimization)]
+        [SuppressMessage("Major Code Smell", "S907:\"goto\" statement should not be used")]
         private static void Search5(Job me)
         {
             Run* run = me._run;
@@ -159,7 +158,6 @@ private static void Search5(Job me)
 
             var allBytes = stackalloc byte[(int)bytesRequiredTotal];
 
-            //var builderAlloc = stackalloc byte[builderByteLength];
             var builderStart = MemoryAlignmentHelper.GetCacheAlignedStart<char>(allBytes);
             *builderStart = (char)0;
             builderStart++;
@@ -179,7 +177,7 @@ private static void Search5(Job me)
             // Row 1 is the first character and so on...
             // And we sneak in an extra row before row0, to ensure that the transposition row offset has room.
             closureArg._transpositionRow = (uint*) 0;
-            closureArg._row0 = editMatrix;// + twiceMaxPlusOne;
+            closureArg._row0 = editMatrix;
             editMatrix--;
             nint x = 0;
             nint diagonal = 0;
@@ -197,6 +195,7 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                 nuint builderDepth = closure._builderDepth;
                 uint* previousRow = closure._row0;
                 uint mp1 = closure._maxPlusOne;
+
                 // This is the value for the column directly before our diagonal stripe.
                 // For the early rows, it's the 0'th column. After that, the value is > maxPlusOne anyways.
                 // TODO: Potentially a place to cut down on an addition or two, if it fits in the branching done above.
@@ -209,8 +208,8 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                 char* firstWithOffset = closure._first;
                 uint to = closure._wordLength;
 
-                //char previousWordCharacter;
                 uint previousRowPreviousColumn;
+
                 // Very predictable branching
                 if (wordArrayOffset > 0)
                 {
@@ -224,7 +223,7 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                     }
                     else
                     {
-                        //TODO: I could move this out of the if-else and make the if do ++ instead of +=. Does this affect latency?
+                        //TODO: Test alternative: move this outside of the conditional, and change the previous addition to +1?
                         transpositionRow++;
                     }
 
@@ -240,6 +239,7 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                 // I tried to arrange these so that the order of operations leaves them as far away as possible from ops affecting their values.
                 // To reduce latency.
                 closure._transpositionRow = transpositionRow;
+
                 // TODO: is this one predictable?
                 if (t2 < to)
                 {
@@ -250,7 +250,6 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
 
                 // Save a read by combining both characters into one register. Used ulong because comparison is best done between two int32
                 // Shift the previous-previous character into the high bits, putting the previous character into the bottom 2.
-                // 00ab -> //b00a | so (uint)edgeCharacter == 0a, (uint) (edgeCharacter >> 48) == 0b
                 // Make sure it's unsigned so that >> 48 will shift 0's
                 ulong edgeCharacter = MathUtils.RotateRight((ulong) *(uint*) (closure._builderStart + builderDepth - 1), 16);
 
@@ -260,16 +259,12 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                 do
                 {
                     uint previousRowCurrentColumn = previousRow[j];
-
                     ulong wordCharacter = MathUtils.RotateRight((ulong) *(uint*) (firstWithOffset + j - 1), 16);
-                    //uint wordCharacter = *(uint*) (firstWithOffset + j - 1);
-
                     if ((uint) edgeCharacter != (uint) wordCharacter)
                     {
                         // Non-branching Min() call here because it's not predictable.
                         currentRowPreviousColumn = MathUtils.Min(currentRowPreviousColumn, previousRowCurrentColumn);
                         uint diagonalEntry;
-                        //if ((uint) edgeCharacter != (uint) (wordCharacter >> 48) || (uint) (edgeCharacter >> 48) != (uint) wordCharacter)
                         // ReSharper disable once ConvertIfStatementToConditionalTernaryExpression
                         if (edgeCharacter != ((wordCharacter >> 48) | (wordCharacter << 48)))
                         {
@@ -291,13 +286,14 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                     }
 
                     currentRow[j] = currentRowPreviousColumn;
-                    // Doing this here removes a pause between ++ and < in (++j < to)
+
+                    // Doing this here to reduce latency between the add and the conditional
                     ++j;
 
                     // Will make `any` negative if currentRowPreviousColumn < maxErrors
                     any |= (int) currentRowPreviousColumn - (int) maxPlusOne;
                     previousRowPreviousColumn = previousRowCurrentColumn;
-                } while ((uint)j < to);
+                } while ((uint) j < to);
 
                 if (any >= 0)
                 {
@@ -315,6 +311,7 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                         string stringResult = new string(bs, 0, (int)newDepth);
                         ulong count = GetWordCount(bs, newDepth, closure._graph);
                         closure._results.Add(stringResult, count);
+
                         // If we followed this branch, the register was overwritten.
                         // Fetch the value from the correct place instead of having the compiler store it on the stack.
                         maxPlusOne = closure._maxPlusOne;
@@ -376,7 +373,9 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                 ulong temp = *(ulong*) (closure._graph._firstChildEdgeIndex + (uint) closure._node);
                 // This one is used an a pointer offset, so it can be nuint. Also, taking only the lower 32 bits.
                 // ReSharper disable once RedundantCast - Just being explicit and clear
+#pragma warning disable IDE0004, S1905 // Remove Unnecessary Cast
                 nuint childEdge = (nuint) (uint) temp;
+#pragma warning restore IDE0004, S1905 // Remove Unnecessary Cast
                 // Upper 32 bits. Not used as an offset, so keep as uint.
                 uint childEdgeEnd = (uint) (temp >> 32);
 
@@ -386,7 +385,6 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
                 }
 
                 // TODO: We have some extra registers to store stuff in, and they're getting push-popped anyways. Load things and prevent re-reading from memory.
-                // TODO: make a BIT64 distinction. We don't have all those extra registers in 32 bit mode.
                 uint* row0 = closure._row0;
                 closure._builderDepth = newDepth;
                 // casting to byte* because otherwise it was doing both shl 1 and shl 2 to double _maxPlusOne and then convert it to pointer moves
@@ -416,6 +414,7 @@ static void MatchCharacter(ref Search3Closure closure, nuint skip)
             MatchCharacter(ref closureArg, 0);
         }
 
+#pragma warning disable S3898 //Value types should implement "IEquatable<T>"
         private readonly ref struct Run
         {
             public readonly char* _input;
@@ -431,6 +430,7 @@ public Run(char* input, uint* toCache, uint wordLength, uint maxPlusOne)
                 _maxPlusOne = maxPlusOne;
             }
         }
+#pragma warning restore S3898 //Value types should implement "IEquatable<T>"
 
         private class Job
         {
@@ -458,6 +458,7 @@ private void SearchPrivate()
         }
 
         [StructLayout(LayoutKind.Explicit)]
+#pragma warning disable S3898 //Value types should implement "IEquatable<T>"
         private readonly struct DawgGraph
         {
             [FieldOffset(0x00)] //48
@@ -488,6 +489,7 @@ public DawgGraph(ushort* reachableTerminalNodes, ulong* wordCounts, uint* firstC
                 _rootNodeIndex = rootNodeIndex;
             }
         }
+#pragma warning restore S3898 //Value types should implement "IEquatable<T>"
 
         [Pure]
         [MethodImpl(MethodImplOptions.AggressiveOptimization)]
@@ -507,12 +509,16 @@ private static ulong GetWordCount(char* word, nuint length, in DawgGraph dawg)
                 char target = *word;
                 word++;
                 // ReSharper disable once RedundantCast - Just being explicit and clear
+#pragma warning disable IDE0004 // Remove Unnecessary Cast
                 nuint i = (nuint) edgeChildIndex[currentNode];
+#pragma warning restore IDE0004 // Remove Unnecessary Cast
                 uint lastChildIndex = edgeChildIndex[currentNode+1];
                 do
                 {
                     // ReSharper disable once RedundantCast - Just being explicit and clear
+#pragma warning disable IDE0004, S1905 // Remove Unnecessary Cast
                     currentNode = (nint)edgeNodeIndex[i];
+#pragma warning restore IDE0004, S1905 // Remove Unnecessary Cast
                     char currentEdgeChar = characters[i];
                     i++;
                     if (currentEdgeChar != target)
@@ -541,6 +547,7 @@ private static ulong GetWordCount(char* word, nuint length, in DawgGraph dawg)
         }
 
         [StructLayout(LayoutKind.Explicit)]
+#pragma warning disable S3898 //Value types should implement "IEquatable<T>"
         private ref struct Search3Closure
         {
             #region CacheLine1
@@ -582,6 +589,7 @@ private ref struct Search3Closure
             public SuggestItemCollection _results;
             #endregion CacheLine3
         }
+#pragma warning restore S3898 //Value types should implement "IEquatable<T>"
 
         public uint WordCount { get; }
 
@@ -695,7 +703,9 @@ public int GetIndex(in string word)
 #pragma warning restore S907 // "goto" statement should not be used
                 }
 
+#pragma warning disable S907 // "goto" statement should not be used
                 goto singleReturnPoint;
+#pragma warning restore S907 // "goto" statement should not be used
 
                 nextIteration:
 #pragma warning disable S1116 // Empty statements should be removed - This statement is important. It allows the label/goto to work.
diff --git a/Portent/Graph/CompressedSparseRowGraph.cs b/Portent/Graph/CompressedSparseRowGraph.cs
@@ -39,17 +39,11 @@ public CompressedSparseRowPointerGraph(Stream stream)
             EdgeToNodeIndex = MemoryChunk.GetArrayAligned<int>(edgeToNodeIndexCount);
             stream.ReadCompressed(EdgeToNodeIndex, edgeToNodeIndexCount);
 
-            GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce;
-            GC.Collect();
-
             var byteLength = stream.ReadCompressedUInt();
             var charLength = stream.ReadCompressedUInt();
             EdgeCharacter = MemoryChunk.GetArrayAligned<char>(charLength);
             stream.ReadUtf8(EdgeCharacter, byteLength, charLength);
 
-            GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce;
-            GC.Collect();
-
             var reachableTerminalNodesCount = stream.ReadCompressedUInt();
             ReachableTerminalNodes = MemoryChunk.GetArrayAligned<ushort>(reachableTerminalNodesCount);
             stream.ReadCompressed(ReachableTerminalNodes, reachableTerminalNodesCount);
@@ -62,7 +56,7 @@ public CompressedSparseRowPointerGraph(Stream stream)
         }
     }
 
-    public sealed class CompressedSparseRowGraph
+    public sealed class CompressedSparseRowGraph : IDisposable
     {
         internal CompressedSparseRowGraph(int rootNodeIndex, uint[] firstChildEdgeIndex, int[] edgeToNodeIndex, char[] edgeCharacter, ushort[] reachableTerminalNodes, ulong[] data, Dictionary<string, ulong> wordCounts)
             : this(rootNodeIndex, firstChildEdgeIndex, edgeToNodeIndex, edgeCharacter, reachableTerminalNodes, data)
@@ -150,5 +144,13 @@ public void Save(string path)
             stream.WriteCompressed(ReachableTerminalNodes);
             stream.WriteCompressed(WordCounts);
         }
+
+        public void Dispose()
+        {
+            GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce;
+#pragma warning disable S1215 // "GC.Collect" should not be called
+            GC.Collect();
+#pragma warning restore S1215 // "GC.Collect" should not be called
+        }
     }
 }
diff --git a/Portent/Graph/PartitionedGraphBuilder.cs b/Portent/Graph/PartitionedGraphBuilder.cs
diff --git a/Portent/Portent.csproj b/Portent/Portent.csproj
diff --git a/Portent/Serialization/StreamExtensions.cs b/Portent/Serialization/StreamExtensions.cs

Original file line number	Diff line number	Diff line change
`@@ -84,8 +84,11 @@ public static Dawg CreateDictionary(string corpusPath, string savePath)`
`84`	`84`	`}`
`85`	`85`	`}`
`86`	`86`
`87`		`- var compressedGraph = builder.AsCompressedSparseRows();`
`88`		`- compressedGraph.Save(savePath);`
	`87`	`+ using (var compressedGraph = builder.AsCompressedSparseRows())`
	`88`	`+ {`
	`89`	`+ compressedGraph.Save(savePath);`
	`90`	`+ }`
	`91`	`+`
`89`	`92`	`using var dawgStream = File.OpenRead(savePath);`
`90`	`93`	`return new Dawg(dawgStream);`
`91`	`94`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,21 +11,18 @@ public static void Main()`
`11`	`11`	`{`
`12`	`12`	`if (Debugger.IsAttached)`
`13`	`13`	`{`
`14`		`- RunForProfiler();`
	`14`	`+ PrintResultCounts();`
`15`	`15`	`}`
`16`	`16`	`else`
`17`	`17`	`{`
`18`	`18`	`BenchmarkRunner.Run<DawgBenchmark>();`
`19`	`19`	`}`
`20`	`20`	`}`
`21`	`21`
`22`		`- private static void RunForProfiler()`
	`22`	`+ private static void PrintResultCounts()`
`23`	`23`	`{`
`24`		`- Console.WriteLine("reading");`
`25`	`24`	`using var benchmark = new DawgBenchmark();`
`26`		`- Console.WriteLine("setup for run");`
`27`	`25`	`benchmark.SetupForRun();`
`28`		`- Console.WriteLine("verify correctness");`
`29`	`26`	`if (!benchmark.VerifyDawgCorrectness())`
`30`	`27`	`{`
`31`	`28`	`throw new InvalidOperationException();`
`@@ -40,7 +37,6 @@ private static void RunForProfiler()`
`40`	`37`	`Console.WriteLine(benchmark.GetTotalResults());`
`41`	`38`	`}`
`42`	`39`
`43`		`- Console.WriteLine("Done, press {ENTER} to continue...");`
`44`	`40`	`Console.ReadLine();`
`45`	`41`	`}`
`46`	`42`	`}`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE`
`29`	`29`	`using System.Runtime.CompilerServices;`
`30`	`30`	`using System.Runtime.InteropServices;`
`31`	`31`	`using System.Threading;`
`32`		`-`
	`32`	`+#pragma warning disable S3453 // Classes should not have only "private" constructors`
`33`	`33`	`namespace Portent`
`34`	`34`	`{`
`35`	`35`	`/// <summary>`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`
`8`	`8`	`namespace Portent`
`9`	`9`	`{`
`10`		`- internal sealed class SuggestItemCollection : IEnumerable<SuggestItem>`
	`10`	`+ internal sealed class SuggestItemCollection : IEnumerable<SuggestItem>, IDisposable`
`11`	`11`	`{`
`12`	`12`	`public SuggestItemCollection(int count)`
`13`	`13`	`{`
`@@ -123,5 +123,11 @@ public void Dispose()`
`123`	`123`	`//Empty - Required by IEnumerator<T>, but nothing to dispose.`
`124`	`124`	`}`
`125`	`125`	`}`
	`126`	`+`
	`127`	`+ public void Dispose()`
	`128`	`+ {`
	`129`	`+ // So useless`
	`130`	`+ _myEnumerator.Dispose();`
	`131`	`+ }`
`126`	`132`	`}`
`127`	`133`	`}`