Follow up on PR #17894 on 7.1 version parsing #17990
Conversation
OmarTawfik
added
the
PR For Personal Review Only
| @@ -1347,6 +1320,28 @@ public void LanguageVersion_DisplayString() | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void LanguageVersion_GetErrorCode() | |||
| [InlineData("6.0", LanguageVersion.CSharp6)] | ||
| [InlineData("7", LanguageVersion.CSharp7)] | ||
| [InlineData("7.0", LanguageVersion.CSharp7)] | ||
| [InlineData("7.1", LanguageVersion.CSharp7_1)] |
There was a problem hiding this comment.
Nice. Do you plan do to similar refactoring on VB side? #Closed
There was a problem hiding this comment.
I'd rather keep this PR focused on the C# version side. Every now and then I find similar mega-tests and I break them down, so I prefer doing them as we go.
If you think there is a wider-spread and they should be dealt with sooner, I can file an issue and start looking into them. #Closed
OmarTawfik
added
Area-Compilers
and removed
PR For Personal Review Only
| </data> | ||
| <data name="ERR_FeatureNotAvailableInVersion7" xml:space="preserve"> | ||
| <value>Feature '{0}' is not available in C# 7. Please use language version {1} or greater.</value> | ||
| <value>Feature '{0}' is not available in C# 7. Please use language version {1} or greater.</value> |
There was a problem hiding this comment.
Why did we remove all of the spaces here? Seems unrelated to this PR. #Resolved
There was a problem hiding this comment.
This is a follow-up on the last PR. ERR_FeatureNotAvailableInVersion7_1 was added and it had this extra space. Then Omar pointed out all the previous versions had the same typo. #Resolved
| <value>Feature '{0}' is not available in C# 7.1. Please use language version {1} or greater.</value> | ||
| </data> | ||
| <data name="ERR_LanguageVersionCannotHaveLeadingZeroes" xml:space="preserve"> | ||
| <value>Specified language version '{0}' cannot have leading zeroes</value> |
There was a problem hiding this comment.
Did we discuss what happens with trailing 0s? Sorry if I missed that part of the email. #Resolved
There was a problem hiding this comment.
Yes but that covers only a single trailing 0. What happens when I enter "7.000"?
The code gives an error for leading zero but not a trailing zeros. Possibly a non-issue but want to make sure it was explicitly considered. #Resolved
There was a problem hiding this comment.
We never accepted them in the first place, so this shouldn't be a breaking change. Am I missing something?
In reply to: 107036325 [](ancestors = 107036325)
There was a problem hiding this comment.
This isn't about a breaking change, it's about symmetry.
/langversion:07
Error: leading zeros not allowed
/langversion:7.00
Invalid language version Seems like this could be a reasonable typo in the new point release world.
Not hard feedback. Just an item I want to make sure we've considered.
There was a problem hiding this comment.
We never accepted trailing zeroes, so that's not a breaking change. Am I missing something? #Resolved
There was a problem hiding this comment.
Good point. My personal opinion is that everything eventually should be disallowed with the latter error message. This exception was done for the breaking change. Will have to think about it.
In reply to: 107037475 [](ancestors = 107037475)
| @@ -805,6 +805,12 @@ public new CSharpCommandLineArguments Parse(IEnumerable<string> args, string bas | |||
| { | |||
| AddDiagnostic(diagnostics, ErrorCode.ERR_SwitchNeedsString, MessageID.IDS_Text.Localize(), "/langversion:"); | |||
| } | |||
| else if (value.StartsWith("0")) | |||
There was a problem hiding this comment.
Please use an explicit comparer here. The default for StartsWith is to use culture sensitive comparisons and I'm unsure if that is correct here. Believe this should be ordinal. #Resolved
Responds to feedback on #17894
cc @jcouv @dotnet/roslyn-compiler @dotnet/roslyn-compat-council