Use C# 12 #996

iamcarbon · 2024-01-30T02:01:43Z

This PR updates C# to version 12, and utilizes a few features:

Utilizes utf-8 literals
Utilizes collection expressions
Replaces a couple byte[] arrays with statically initialized data
Removes an unnecessary cast

iamcarbon · 2024-01-30T02:02:10Z

@jstedfast Ready for review / feedback. Let me know what you think of these new syntax features.

jstedfast

See individual code comments for details.

jstedfast · 2024-01-30T14:38:15Z

MimeKit/Encodings/Base64Encoder.cs

-			0x67, 0x68, 0x69, 0x6A, 0x6B, 0x6C, 0x6D, 0x6E, 0x6F, 0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76,
-			0x77, 0x78, 0x79, 0x7A, 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x2B, 0x2F
-		};
+		static ReadOnlySpan<byte> base64_alphabet => "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"u8;



Do we really need the u8 here? All of the characters fit in ASCII, so I would imagine it wouldn't.

As much as I am an old dog that is stuck in his old ways (long-time C programmer who is generally a stickler for more C-like syntax), I definitely prefer "ABCDEF..." over 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, ...

👍

The u8 postfix converts 16-bit wide chars to 8 bit wide ASCII bytes. If we omit the "u8", we'd be changing this to ReadOnlySpan<char>.

Ugh, gotcha. The u8 at first glance looks like a typo at the end of the string which bugs me 😆

jstedfast · 2024-01-30T14:38:59Z

MimeKit/Encodings/HexEncoder.cs

-			0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, // '0' -> '7'
-			0x38, 0x39, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, // '8' -> 'F'
-		};
+		static ReadOnlySpan<byte> hex_alphabet => "0123456789ABCDEF"u8;



Same as base64_alphabet

Same as above. The u8 postfix turns the sequence into a ASCII bytes (instead of chars).

jstedfast · 2024-01-30T14:40:44Z

MimeKit/ExperimentalMimeParser.cs

@@ -53,7 +53,7 @@ public class ExperimentalMimeParser : MimeReader, IMimeParser
 		int mboxMarkerLength;

 		// Current MimeMessage/MimeEntity Header state
-		readonly List<Header> headers = new List<Header> ();
+		readonly List<Header> headers = [];
 		byte[] preHeaderBuffer;
 		int preHeaderLength;



This syntax change I'm not sure I like. It might grow on me like some of the past syntax changes you've made have grown on me, though. Same goes for all of the [] changes made.

I'll try proposing again in year. :)

jstedfast · 2024-01-30T14:41:43Z

MimeKit/Header.cs

@@ -964,7 +964,7 @@ static byte[] EncodeReferencesHeader (ParserOptions options, FormatOptions forma

 		static bool IsWhiteSpace (char c)
 		{
-			return c == ' ' || c == '\t' || c == '\r' || c == '\n';
+			return c is ' ' or '\t' or '\r' or '\n';
 		}



I think you suggested this change in the past and I think I rejected it, but the syntax has grown on me...

So 👍

Yay. I'll get a clean PR with just the changes you like.

jstedfast · 2024-01-30T14:44:34Z

MimeKit/Header.cs

@@ -1744,7 +1744,7 @@ internal static unsafe bool TryParse (ParserOptions options, byte* input, int le
 				}
 			} else {
 				// Note: The only way to get here is if we have an invalid header, in which case the entire 'header' is stored as the 'field'.
-				value = Array.Empty<byte> ();
+				value = [];
 			}


of all of the [] syntax changes, I think I like this one the least because it's not obvious if this is allocating or not and the point of using Array.Empty<byte> () is to avoid allocations.

This one I am going to specifically reject.

jstedfast · 2024-01-30T14:47:11Z

MimeKit/IO/Filters/ArmoredFromFilter.cs

@@ -41,7 +41,7 @@ namespace MimeKit.IO.Filters {
 	/// </remarks>
 	public class ArmoredFromFilter : MimeFilterBase
 	{
-		static readonly byte[] From = { (byte) 'F', (byte) 'r', (byte) 'o', (byte) 'm', (byte) ' ' };
+		static ReadOnlySpan<byte> From => [(byte) 'F', (byte) 'r', (byte) 'o', (byte) 'm', (byte) ' '];



There's 2 changes here, one that updates the code to use ReadOnlySpan<byte> and one that updates the code to use []-style syntax.

What if we used:

static ReadOnlySpan<byte> From => "From ";

I think that'd be clearer and more concise.

And actually, would it make sense to do this?

static readonly ReadOnlySpan<byte> From => "From ";

We'd still need the u8 suffix to covert to ASCII bytes.

jstedfast · 2024-01-30T14:47:45Z

MimeKit/IO/Filters/MboxFromFilter.cs

@@ -38,7 +38,7 @@ namespace MimeKit.IO.Filters {
 	/// </remarks>
 	public class MboxFromFilter : MimeFilterBase
 	{
-		static readonly byte[] From = { (byte) 'F', (byte) 'r', (byte) 'o', (byte) 'm', (byte) ' ' };
+		static ReadOnlySpan<byte> From => [(byte) 'F', (byte) 'r', (byte) 'o', (byte) 'm', (byte) ' '];
 		bool midline;

 		/// <summary>


Lets do the same as ArmoredFromFilter

jstedfast · 2024-01-30T14:57:45Z

MimeKit/InternetAddress.cs

@@ -373,7 +373,7 @@ internal static bool TryParseLocalPart (byte[] text, ref int index, int endIndex
 			return true;
 		}

-		static ReadOnlySpan<byte> CommaGreaterThanOrSemiColon => new[] { (byte) ',', (byte) '>', (byte) ';' };
+		static ReadOnlySpan<byte> CommaGreaterThanOrSemiColon => [(byte)',', (byte) '>', (byte) ';'];

 		internal static bool TryParseAddrspec (byte[] text, ref int index, int endIndex, ReadOnlySpan<byte> sentinels, RfcComplianceMode compliance, bool throwOnError, out string addrspec, out int at)


I'm not sure the [] gets us any more clarity over {}, but I think we have another opportunity here for the string syntax that would at least save us the ugliness of the (byte) casts (which are probably technically not needed?)

static readonly ReadOnlySpan<byte> CommaGreaterThanOrSemiColon => ",>;";

My only hesitation with my own proposed change is that it would look like its usage might be as a string rather than as individual bytes (which are meant to be treated as alternatives rather than a sequence). That said, it's a minor point of uncertainty that the CommaGreaterThanOrSemiColon constant name probably remedies.

jstedfast · 2024-01-30T15:05:03Z

MimeKit/ParameterList.cs

 					}
 				} else {
 					// When no charset is specified, it should be safe to assume US-ASCII...
 					// but we all know what assume means, right??
 					encoding = Encoding.GetEncoding (28591); // iso-8859-1
-					decoder = (Decoder) encoding.GetDecoder ();
+					decoder = encoding.GetDecoder ();
 				}


We should definitely get rid of these casts. I think they are a relic of when I had to depend on my Portable.Text.Encoding library for charset support on Windows Phone back when I was trying to support that platform.

And therefore I had a using Decoder = Portable.Text.Encoding.Decoder;

jstedfast · 2024-01-30T15:22:41Z

MimeKit/Utils/ParseUtils.cs

@@ -390,7 +390,7 @@ public static bool TryParseDomain (byte[] text, ref int index, int endIndex, Rea
 			return TryParseDotAtom (text, ref index, endIndex, sentinels, throwOnError, "domain", out domain);
 		}

-		static ReadOnlySpan<byte> GreaterThanOrAt => new[] { (byte) '>', (byte) '@' };
+		static ReadOnlySpan<byte> GreaterThanOrAt => [(byte) '>', (byte) '@'];



This could also potentially use the static readonly ReadOnlySpan<byte> GreaterThanOrAt => ">@"; syntax.

I was on the fence on whether converting to a string also lost some meaning, but agree this can be overlooked when the variable is well named. If we make this change, we still need the u8 suffix.

iamcarbon · 2024-01-30T17:49:22Z

@jstedfast Thanks for the feedback. Let me know your thoughts on the u8 suffix, and I'll put out new PR with just the approved changes.

jstedfast · 2024-01-30T20:58:35Z

A few things:

Based on some quick playing around, it looks like we can't add the readonly modifier to these static ReadOnlySpan<byte> constants. No big deal there, I was just wondering but now I have an answer.
Lets do the u8 changes. As much as I cringe at that u8 suffix to the strings, I think "01234567689ABCDEF"u8 (for example) is a lot more clear than the array of hexadecimal byte values.

iamcarbon added 5 commits January 29, 2024 17:54

Use C# 12

cf12384

Use statically initialized data

f352b4d

Use utf-8 literals

433d209

Use collection expressions

d16322f

Remove unnecessary cast

8ac9951

iamcarbon temporarily deployed to ci January 30, 2024 02:01 — with GitHub Actions Inactive

iamcarbon had a problem deploying to ci January 30, 2024 02:01 — with GitHub Actions Failure

iamcarbon temporarily deployed to ci January 30, 2024 02:01 — with GitHub Actions Inactive

jstedfast requested changes Jan 30, 2024

View reviewed changes

iamcarbon mentioned this pull request Jan 30, 2024

Improve Code Quality #997

Merged

iamcarbon closed this Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use C# 12 #996

Use C# 12 #996

iamcarbon commented Jan 30, 2024

iamcarbon commented Jan 30, 2024

jstedfast left a comment

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024 •

edited by jstedfast

Loading

jstedfast Jan 30, 2024

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024

jstedfast Jan 30, 2024

jstedfast Jan 30, 2024

jstedfast Jan 30, 2024

jstedfast Jan 30, 2024

iamcarbon Jan 30, 2024

iamcarbon commented Jan 30, 2024

jstedfast commented Jan 30, 2024

Use C# 12 #996

Use C# 12 #996

Conversation

iamcarbon commented Jan 30, 2024

iamcarbon commented Jan 30, 2024

jstedfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamcarbon Jan 30, 2024 • edited by jstedfast Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamcarbon commented Jan 30, 2024

jstedfast commented Jan 30, 2024

iamcarbon Jan 30, 2024 •

edited by jstedfast

Loading