Skip to content

Support raw UTF8 String Literals#59390

Merged
AlekseyTs merged 3 commits into
dotnet:features/Utf8StringLiteralsfrom
AlekseyTs:Utf8StringLiterals_10
Feb 10, 2022
Merged

Support raw UTF8 String Literals#59390
AlekseyTs merged 3 commits into
dotnet:features/Utf8StringLiteralsfrom
AlekseyTs:Utf8StringLiterals_10

Conversation

@AlekseyTs
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs commented Feb 8, 2022 •
edited by jcouv
Loading

Relates to test plan #58848

@AlekseyTs AlekseyTs added Area-Compilers Feature - Utf8StringLiterals PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Feb 8, 2022
@AlekseyTs AlekseyTs requested review from a team, RikkiGibson and cston February 8, 2022 23:16
@AlekseyTs AlekseyTs marked this pull request as ready for review February 8, 2022 23:16
This was referenced Feb 9, 2022
Closed
Closed
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Feb 9, 2022

@cston, @RikkiGibson, @dotnet/roslyn-compiler Please review.

@RikkiGibson RikkiGibson self-assigned this Feb 9, 2022
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Feb 10, 2022

@cston, @dotnet/roslyn-compiler For the second review.

cston
cston reviewed Feb 10, 2022
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Syntax/Parsing/LineSpanDirectiveParsingTests.cs
N(SyntaxKind.EndOfDirectiveToken);
}
EOF();
}
Copy link
Copy Markdown
Contributor

@cston cston Feb 10, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

Please add tests for enhanced line directives with UTF8 string literals and UTF8 raw string literals:

#line (1, 2)-(3, 4) ""file.cs""u8
#line (1, 2)-(3, 4) """file.cs"""u8
``` #Resolved

Copy link
Copy Markdown
Contributor Author

@AlekseyTs AlekseyTs Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for enhanced line directives with UTF8 string literals and UTF8 raw string literals:

If you insist, I can add the suggested test. However, I do not see much benefits in testing every single directive flavor. The goal is to test how we scan literals in context of directives, i.e. to confirm that we do not recognize the suffix. I believe the existing tests are sufficient to cover the scenario.

Please let me know if you still want me to add the tests.

cston
cston reviewed Feb 10, 2022
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt Outdated
@@ -32,3 +32,5 @@ static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.InterpolatedStringExpression(
Microsoft.CodeAnalysis.CSharp.Conversion.IsUtf8StringLiteral.get -> bool
Copy link
Copy Markdown
Contributor

@cston cston Feb 10, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utf8

Consider changing to IsUTF8StringLiteral for consistency with public API members below. #Resolved

Copy link
Copy Markdown
Contributor Author

@AlekseyTs AlekseyTs Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing to IsUTF8StringLiteral for consistency with public API members below.

If there are no objections, I prefer doing this in a separate PR.

cston
cston reviewed Feb 10, 2022
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Syntax/Parsing/UTF8StringLiteralsParsingTests.cs Outdated

[Theory]
[InlineData("u80")]
[InlineData("U80")]
Copy link
Copy Markdown
Contributor

@cston cston Feb 10, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be "u8" and "U8" rather than "u80" and "U80"? Same question for other Interpolation_0*() tests below. #Resolved

Copy link
Copy Markdown
Contributor Author

@AlekseyTs AlekseyTs Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be "u8" and "U8" rather than "u80" and "U80"?

Yes, good catch. I'll fix.

@AlekseyTs AlekseyTs enabled auto-merge (squash) February 10, 2022 23:02
@AlekseyTs AlekseyTs merged commit 414ebea into dotnet:features/Utf8StringLiterals Feb 10, 2022
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Feb 11, 2022

Note to self: IDE code that works with raw string literals must not presume that a syntactically valid raw string starts and ends with the exact same sequence of quotes. e.g. """x"""u8 is legal. Code in the ide that finds the content portion needs to be resilient to u8/U8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RikkiGibson RikkiGibson

Labels

Area-Compilers Feature - Utf8StringLiterals

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AlekseyTs @CyrusNajmabadi @RikkiGibson @cston