antlr4
antlr4 copied to clipboard
C#: remove redundant functionality and upgrade Windows .NET to 462
- Remove redundant functionality (use native .NET api instead of hand-written classes and methods)
- Upgrade net45 to net462 since net45 doesn't support
Array.Empty<T>()
and it's too old
@ericvergnaud I hope it's much easier for you to review this pull request (it's mostly manual refactoring).
Nice work. I don't see anything scary in there other than we're moving some files around. Most of the code does look like just clean up. Happy if @ericvergnaud could take a quick scan.
@ericvergnaud I hope it's much easier for you to review this pull request (it's mostly manual refactoring).
Well not sure why you insist on making big PRs. I'm reviewing, but this should have been about dropping Sharpen only.
- Remove redundant functionality (use native .NET api instead of hand-written classes and methods)
- Upgrade net45 to net462 since net45 doesn't support
Array.Empty<T>()
and it's too old if we move, it should be to net472. Net462 is no longer supported
if we move, it should be to net472
- I don't understand completely if net462 is no longer supported. According to the table, net462 and net472 have the same status: https://en.wikipedia.org/wiki/.NET_Framework_version_history#Overview
- Some users still need net462, see https://github.com/antlr/antlr4/issues/3212 @appel1 is that true?
- Some users still need net462, see Provide .NET Framework target in the csharp nuget package #3212 @appel1 is that true?
@KvanTTT Yes. At least for now. Hopefully it'll change next year once the extended support for Windows 7 ends.
As I understand it there's no explicit end date for 4.6.2 because it follows its parent OS. https://docs.microsoft.com/en-us/lifecycle/products/microsoft-net-framework
if we move, it should be to net472
- I don't understand completely if net462 is no longer supported. According to the table, net462 and net472 have the same status: https://en.wikipedia.org/wiki/.NET_Framework_version_history#Overview
- Some users still need net462, see Provide .NET Framework target in the csharp nuget package #3212 @appel1 is that true?
See https://dotnet.microsoft.com/en-us/download/dotnet-framework/net46. C# users can stick to older versions since we're not fixing any bug.
if we move, it should be to net472
- I don't understand completely if net462 is no longer supported. According to the table, net462 and net472 have the same status: https://en.wikipedia.org/wiki/.NET_Framework_version_history#Overview
- Some users still need net462, see Provide .NET Framework target in the csharp nuget package #3212 @appel1 is that true?
See https://dotnet.microsoft.com/en-us/download/dotnet-framework/net46. C# users can stick to older versions since we're not fixing any bug.
That's 4.6 not 4.6.2.
4.6 is no longer supported. 4.6.2 very much is.
C# users can stick to older versions since we're not fixing any bug.
If we follow this logic, we had to deprecate Python 2 a long time ago since it was discontinued in 2020, but we still support it: https://www.python.org/doc/sunset-python-2/
Also, we changed the ATN format in ANTLR 4.10, new generated lexers and parser are not back compatible.
if we move, it should be to net472
- I don't understand completely if net462 is no longer supported. According to the table, net462 and net472 have the same status: https://en.wikipedia.org/wiki/.NET_Framework_version_history#Overview
- Some users still need net462, see Provide .NET Framework target in the csharp nuget package #3212 @appel1 is that true?
See https://dotnet.microsoft.com/en-us/download/dotnet-framework/net46. C# users can stick to older versions since we're not fixing any bug.
That's 4.6 not 4.6.2.
4.6 is no longer supported. 4.6.2 very much is.
Fair enough
C# users can stick to older versions since we're not fixing any bug.
If we follow this logic, we had to deprecate Python 2 a long time ago since it was discontinued in 2020, but we still support it: https://www.python.org/doc/sunset-python-2/
Also, we changed the ATN format in ANTLR 4.10, new generated lexers and parser are not back compatible.
That's not the best hasty decision we ever made, that breaking change did not create any practical value, and we had to provide specific support which would have been avoided had we done this change more calmly.
My principles ave very simple:
- don't change code that works unless it creates value (other than 'improving' style)
- make small changes, they are easier to revert in case something goes wrong
- support legacy versions unless something is in the way
Sounds like this is too big of a change for no outward improvement. I agree that refactoring PRs should be viewed skeptically. Sometimes it's worth it. The atn int vs string has been a thorn in my side forever and was a big difference / confusion point between target code.
Ok, i'm holding off on merging this one. Maybe we split it up?
I can push !fixup
commits to make further review easier (review only changes instead of entire PR). Before the final merging, I'll squash !fixup
commit with ordinary ones.
I think @ericvergnaud's comment about risk-reward equation is valid though. Refactoring has a high risk and little reward in this case. Yes, it's much better to have "clean" code but...