Code cleanup and formatting for System.Memory src and test by ahsonkhan · Pull Request #28875 · dotnet/corefx

ahsonkhan · 2018-04-06T03:38:10Z

Each set of changes is a separate commit.

Related PR on the coreclr side: dotnet/coreclr#17451

cc @dotnet/corefxlab-contrib, @stephentoub, @jkotas

…ase characters: s_maxBufferSize

…ase characters: e0

joshfree · 2018-04-06T05:48:11Z

src/System.Memory/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Date.R.cs

-                    case 0x5468752c /* 'Thu,' */: dayOfWeek = DayOfWeek.Thursday; break;
-                    case 0x4672692c /* 'Fri,' */: dayOfWeek = DayOfWeek.Friday; break;
-                    case 0x5361742c /* 'Sat,' */: dayOfWeek = DayOfWeek.Saturday; break;
+                    case 0x53756E2c /* 'Sun,' */:


I think this change makes the code less readable.

joshfree · 2018-04-06T05:48:27Z

src/System.Memory/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Date.R.cs

-                    case 0x4f637420 /* 'Oct ' */ : month = 10; break;
-                    case 0x4e6f7620 /* 'Nov ' */ : month = 11; break;
-                    case 0x44656320 /* 'Dec ' */ : month = 12; break;
+                    case 0x4a616e20 /* 'Jan ' */ :


same for this switch change

joshfree · 2018-04-06T05:51:54Z

20:59:42 System\ValueTuple\ValueTuple.cs(249,72): error CS0117: 'HashHelpers' does not contain a definition for 'RandomSeed' [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.ValueTuple\src\System.ValueTuple.csproj]
20:59:42     0 Warning(s)
20:59:42     1 Error(s)

While it's great to cleanup code formatting, this type of change is unnecessary risk to the RC milestone. Please hold off on merging this until master becomes 2.2 later this month.

stephentoub · 2018-04-06T13:14:48Z

src/Common/src/System/Numerics/Hashing/HashHelpers.cs

    internal static class HashHelpers
    {
-        public static readonly int RandomSeed = Guid.NewGuid().GetHashCode();
+        public static readonly int s_randomSeed = Guid.NewGuid().GetHashCode();


We don't use the s_ naming for public fields.

I didn't know that. We should add that as an exception to our coding style guidelines.

ghost · 2018-04-06T14:31:55Z

src/System.Memory/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Float.cs

                return true;

-            return TryParseAsSpecialFloatingPoint<double>(source, double.PositiveInfinity, double.NegativeInfinity, double.NaN, out value, out bytesConsumed);
+            return TryParseAsSpecialFloatingPoint(source, double.PositiveInfinity, double.NegativeInfinity, double.NaN, out value, out bytesConsumed);


The float version of this method specifies the generic type explicitly - they should stay consistent. (And I'd prefer you keep it explicit. We don't need to jump to accomodate every time the VS IDE whines that the compiler can figure something out. We don't write source code just for the compiler's sake.)

ghost · 2018-04-06T14:53:45Z

src/System.Memory/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Date.G.cs

 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

-using System.Diagnostics;


Personally, I don't think we need go out of our way to remove using System.Diagnostics. Encouraging the use of assertions by removing friction from its use is more valuable than removing every bit of white-font from the IDE.

joshfree

Approved for 2.2

stephentoub · 2018-04-17T13:10:38Z

src/System.Memory/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Date.L.cs

            }

-            var dayAbbrev = DayAbbreviationsLowercase[(int)value.DayOfWeek];
+            var dayAbbrev = s_dayAbbreviationsLowercase[(int)value.DayOfWeek];


var => whatever type this is... same for the other such cases

stephentoub

Other than the remaining feedback, LGTM. Please squash when merging.

stephentoub · 2018-05-04T16:34:40Z

@ahsonkhan, are you still working on this?

ahsonkhan · 2018-05-04T19:27:54Z

are you still working on this?

I will refresh the PR to address the feedback in a couple of days. Thanks for the reminder.

…pCode

ahsonkhan · 2018-05-21T22:00:15Z

@dotnet-bot test Windows x64 Debug Build

ahsonkhan · 2018-05-22T00:48:07Z

@dotnet-bot test NETFX x86 Release Build
@dotnet-bot test Windows x64 Debug Build

ahsonkhan · 2018-05-22T22:54:10Z

@dotnet-bot test Windows x64 Debug Build

…refx#28875) * Cleanup code formatting. * IDE0001 C# Name can be simplified. * IDE0013 C# Name can be simplified. * IDE1006 C# Naming rule violation: Missing prefix: 's_' * C# Naming rule violation: Missing prefix: 's_' * Remove unnecessary using statements. * IDE1006 C# Naming rule violation: These words must begin with upper case characters: s_maxBufferSize * Code formatting of test files. * Cleanup use of pragma warning disable directives in tests. * IDE1005 C# Delegate invocation can be simplified. * IDE1006 C# Naming rule violation: These words must begin with upper case characters: e0 * Fix use of RandomSeed and TestEnum values due to variable rename * Address PR feedback Commit migrated from dotnet/corefx@84f5ac7

ahsonkhan added 11 commits April 5, 2018 20:03

Cleanup code formatting.

0302314

IDE0001 C# Name can be simplified.

56dc564

IDE0013 C# Name can be simplified.

b80a039

IDE1006 C# Naming rule violation: Missing prefix: 's_'

b217249

C# Naming rule violation: Missing prefix: 's_'

71a9dbc

Remove unnecessary using statements.

7e33e4b

IDE1006 C# Naming rule violation: These words must begin with upper c…

768263a

…ase characters: s_maxBufferSize

Code formatting of test files.

f269fea

Cleanup use of pragma warning disable directives in tests.

baec6db

IDE1005 C# Delegate invocation can be simplified.

e27e682

IDE1006 C# Naming rule violation: These words must begin with upper c…

109faa7

…ase characters: e0

ahsonkhan added the area-System.Memory label Apr 6, 2018

ahsonkhan self-assigned this Apr 6, 2018

ahsonkhan mentioned this pull request Apr 6, 2018

Code cleanup and formatting for System.Memory files. dotnet/coreclr#17451

Merged

joshfree reviewed Apr 6, 2018

View reviewed changes

joshfree added this to the 2.2.0 milestone Apr 6, 2018

Fix use of RandomSeed and TestEnum values due to variable rename

badf7f6

stephentoub reviewed Apr 6, 2018

View reviewed changes

stephentoub approved these changes Apr 6, 2018

View reviewed changes

ghost reviewed Apr 6, 2018

View reviewed changes

joshfree approved these changes Apr 6, 2018

View reviewed changes

KrzysztofCwalina approved these changes Apr 10, 2018

View reviewed changes

ahsonkhan added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 11, 2018

stephentoub reviewed Apr 17, 2018

View reviewed changes

stephentoub approved these changes Apr 17, 2018

View reviewed changes

stephentoub removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 11, 2018

ahsonkhan added 2 commits May 21, 2018 13:34

Merge branch 'master' of https://github.com/dotnet/corefx into Cleanu…

f6208e2

…pCode

Address PR feedback

129a0b6

ahsonkhan merged commit 84f5ac7 into dotnet:master May 23, 2018

ahsonkhan deleted the CleanupCode branch May 23, 2018 00:31

Conversation

ahsonkhan commented Apr 6, 2018

Uh oh!

joshfree Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

joshfree Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

joshfree commented Apr 6, 2018

Uh oh!

stephentoub Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

joshfree left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented May 4, 2018

Uh oh!

ahsonkhan commented May 4, 2018

Uh oh!

ahsonkhan commented May 21, 2018

Uh oh!

ahsonkhan commented May 22, 2018

Uh oh!

ahsonkhan commented May 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants