icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
101 stars 13 forks source link

Simplify `decode_sequence` function. #3925

Closed InsertCreativityHere closed 6 months ago

InsertCreativityHere commented 6 months ago

This PR untangles and simplifies the decode_sequence function. Before this PR, it was (probably) the longest function in slicec-cs at 153 lines long. Now it's only 63 (and easier to read too).

I don't expect the changes to this function to be reviewable, but I'm pretty sure no logic has been altered. So it's fine! 😉

What is worth reviewing are the smaller changes it makes to FunctionCallBuilder:

externl commented 6 months ago

💭 You should do a before and after diff of the generated code.

InsertCreativityHere commented 6 months ago

I ran a clean build of main, and copied the src, examples, and tests directories into a new folder and committed them. Then I ran a clean build of this PR, and copied the same folders.

git diff says this is the only change:

index 6fc3d3b..331f9d3 100644
--- b/tests/ZeroC.Slice.Tests/generated/SequenceDecodingTests.cs            // Main
+++ a/tests/ZeroC.Slice.Tests/generated/SequenceDecodingTests.cs            // My PR
@@ -31,7 +31,8 @@ public partial record struct BoolS
     /// <param name="decoder">The Slice decoder.</param>
     public BoolS(ref SliceDecoder decoder)
     {
+        this.Values = decoder.DecodeSequence<bool>(
+            checkElement: SliceDecoder.CheckBoolValue);
-        this.Values = decoder.DecodeSequence<bool>(checkElement: SliceDecoder.CheckBoolValue);
     }

     /// <summary>Encodes the fields of this struct with a Slice encoder.</summary>

Well, technically, a bunch of hashes also changed in various obj folders and .cache files. But who cares.

This change makes sense. Before, we just hardcoded this line as a special case. Now that we're using a builder, it will always play it safe and put arguments on their own line.

I'm actually surprised there aren't more changes, since I altered the function builder to not emit newlines for empty function calls. I guess we never generate empty function calls.