Modernization - #149
Conversation
…ation supports; and (c) models member protection optimization.
kingcean
commented
Feb 6, 2025
mmanela
commented
Feb 16, 2025
I will review soon. Thanks for the contribution |
| #if !NET_TOO_OLD_VER | ||
| public IReadOnlyList<string> Chunk(ReadOnlySpan<char> text) | ||
| { | ||
| // MemoryExtensions.Split(text, lineSeparators, StringSplitOptions.None); |
There was a problem hiding this comment.
What is this commented out code for?
| <ItemGroup> | ||
| <PackageReference Include="BenchmarkDotNet" Version="0.13.12" /> | ||
| <PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" Version="0.13.12" /> | ||
| <PackageReference Include="BenchmarkDotNet" /> |
There was a problem hiding this comment.
@kingcean Can you run benchmarks before before and after the changes and add the results to the PR description? I want to see if we have a measurable impact with this change. We may need to make this benchmark more interesting though
There was a problem hiding this comment.
It would be also good to use this to analyze what else we can improve.
| <AssemblyVersion>1.5.0.0</AssemblyVersion> | ||
| <FileVersion>1.5.0.0</FileVersion> | ||
| <AssemblyVersion>1.6.0.0</AssemblyVersion> | ||
| <FileVersion>1.6.0.0</FileVersion> |
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition="'$(TargetFramework)' == 'netstandard1.0' OR '$(TargetFramework)' == 'net45' OR '$(TargetFramework)' == 'net40' OR '$(TargetFramework)' == 'net35'"> | ||
| <DefineConstants>NET_TOO_OLD_VER</DefineConstants> |
There was a problem hiding this comment.
Can you update the github actions test run to ensure we test both an older and a newer version? Not sure how hard this is, but would be nice to ensure things functions properly with and without the ifdef being true.
mmanela
commented
Mar 22, 2025
@kingcean are you still interested in moving this forward? |
kingcean
commented
Apr 24, 2025
Sorry for the late reply, I will find time to follow up later. |
damienhoneyford
commented
May 11, 2026
Not sure if this PR is going anywhere as it's been sat for a long time now, but scanning through the changes to the core library, it only appears to have added the capability to pass a Realistically though, it's not possible to simply rewrite it to use Span/ReadOnlySpan internally as they're 'ref structs' and can't be stored on the heap (can't use as class fields or within lists, etc.). The best optimisation would be to use StringSegment internally, which would remove the allocations from the sub-strings, but would still be a breaking change as it would require changes to the public API surface, and it still requires at least .NET Standard 2.0. |
This PR is a significant change of core library
DiffPlex.ReadOnlySpan<char>supports to differ and its accessories. At the same time, update the TFM to enable this feature on the available targets. This type is always used for some high performance scenarios to decrease unnecessary memory costs.System.Text.Jsonfor older TFM platforms, e.g. on .NET Frameworks. And please note this feature is disabled on .NET Strandard 1.0 because of not supported.