Closed gfoidl closed 1 year ago
Hi @gfoidl,
Sorry for acting so quickly 😆. I was considering some of the reductions you applied here, however I was very much satisfied with the fact someone else did most of the work on this. I fully agree on adding the System.Memory dependency though and making these improvements. Again your keen eye spotted some nice optimizations (the Write
method naming).
I have to look into the build environment, not sure what platform we use on AppVeyor, which is what's used to deploy the package as well. I recently did my first GitHub actions workflow to publish to NuGet (VisconFactoryIntelligence/AdsClient/blob/main/.github/workflows/dotnet.yml, which also isn't actually based on VS2022, but I guess this works out with dotnet as well).
Anyway, thanks for your time and feedback, will merge and release this as well.
Unfortunately https://github.com/S7NetPlus/s7netplus/pull/482 got merged too fast, so I couldn't have a look and provide my feedback over there, thus here's a PR with my suggestions 😉
For targets before .NET 5 the System.Memory package is used, which provides Span, and Memory for these targets So the code can be simplified a bit by re-using parts of the code
internal
MemoryStreamExtension.WriteBytes
renamed toMemoryStreamExtension.Write
so that the usage as extension solely becomesstream.Write
, so for .NET 5 (when built with VS 2022 as CI does ?!) the C# compiler can avoid some allocations by refering to assembly's static data segment, namely for patterns likepackage.Write(new byte[] { 0x1, 0x2, ... }
. See sharplab example for this (thePrivateImplementationDetails
refers to static data, no further allocation))