antlr4 icon indicating copy to clipboard operation
antlr4 copied to clipboard

C#: remove redundant functionality and upgrade Windows .NET to 462

Open KvanTTT opened this issue 1 year ago • 14 comments

  • 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

KvanTTT avatar Sep 02 '22 16:09 KvanTTT

@ericvergnaud I hope it's much easier for you to review this pull request (it's mostly manual refactoring).

KvanTTT avatar Sep 02 '22 17:09 KvanTTT

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.

parrt avatar Sep 02 '22 17:09 parrt

@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.

ericvergnaud avatar Sep 03 '22 12:09 ericvergnaud

  • 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

ericvergnaud avatar Sep 03 '22 12:09 ericvergnaud

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?

KvanTTT avatar Sep 03 '22 12:09 KvanTTT

@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

appel1 avatar Sep 03 '22 13:09 appel1

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.

ericvergnaud avatar Sep 03 '22 13:09 ericvergnaud

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.

appel1 avatar Sep 03 '22 13:09 appel1

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.

KvanTTT avatar Sep 03 '22 13:09 KvanTTT

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

ericvergnaud avatar Sep 03 '22 13:09 ericvergnaud

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

ericvergnaud avatar Sep 03 '22 13:09 ericvergnaud

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?

parrt avatar Sep 03 '22 17:09 parrt

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.

KvanTTT avatar Sep 03 '22 18:09 KvanTTT

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...

parrt avatar Sep 03 '22 18:09 parrt