Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added a new ConversionKind for Interpolated string #20416

Merged
merged 2 commits into from
Jun 29, 2017
Merged

Added a new ConversionKind for Interpolated string #20416

merged 2 commits into from
Jun 29, 2017

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jun 23, 2017

This adds support for interpolated strings to our public ConversionKind enum. I added support for C# as well. I'm going to hold off on implementing VB because of this issue: #20415. I'll fix that as part of a larger addition of tests for VB conversions, as part of the work for #17590. This PR fixes #20046. Tagging @dotnet/analyzer-ioperation @AlekseyTs for review.

@333fred 333fred requested review from AlekseyTs and mavasani June 23, 2017 00:36
@333fred
Copy link
Member Author

333fred commented Jun 23, 2017

@dotnet-bot retest ubuntu_14_debug_prtest please

Retest by request from @cston, link to the failed build: https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/ubuntu_14_debug_prtest/114/console

   Microsoft.CodeAnalysis.CSharp.UnitTests.Declarations.SourcePlusMetadataTests.InterfaceName [FAIL]
17:44:53       Assert.Equal() Failure
17:44:53       Expected: System.IComparable
17:44:53       Actual:   System.IComparable
17:44:53       Stack Trace:
17:44:53             at Microsoft.CodeAnalysis.CSharp.UnitTests.Declarations.SourcePlusMetadataTests.InterfaceName()

This was referenced Jun 23, 2017
Closed
Closed
@333fred
Copy link
Member Author

333fred commented Jun 23, 2017

Ping @dotnet/analyzer-ioperation?

@333fred
Copy link
Member Author

333fred commented Jun 23, 2017

@dotnet/roslyn-compiler @AlekseyTs, can I get a couple of reviews please?

cston
cston reviewed Jun 23, 2017
View reviewed changes
src/Compilers/Core/Portable/Operations/ConversionKind.cs Outdated
@@ -31,6 +31,11 @@ public enum ConversionKind
/// </summary>
OperatorMethod = 0x5,
/// <summary>
/// Conversion is defined by the underlying type, and is created when an interoplated
/// string expression is being converted to an IFormattable or FormattableString.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: interpolated

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@cston
Copy link
Member

cston commented Jun 23, 2017

LGTM

@333fred
Copy link
Member Author

333fred commented Jun 26, 2017

Ping @AlekseyTs

AlekseyTs
AlekseyTs reviewed Jun 26, 2017
View reviewed changes
src/Compilers/Core/Portable/Operations/ConversionKind.cs
/// Conversion is defined by the underlying type, and is created when an interpolated
/// string expression is being converted to an IFormattable or FormattableString.
/// </summary>
InterpolatedString = 0x6,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

InterpolatedString = 0x6, [](start = 8, length = 25)

Looking at the set of members in this enum, I am not sure if we should be adding InterpolatedString as a member. Should we simply use Basic/CSharp kind instead? For example, we don't try to be more specific about other conversions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we decided in the design group meeting that since this conversion exists in the spec for both languages, we should make this one of the public types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember that we have made this decision, but I am not certain we actually looked at the list of existing kinds in the enum, which could lead us to a different decision. Anyway, we can go with the change as it is, but please bring this to design meeting to confirm that this is what we want.

AlekseyTs
AlekseyTs reviewed Jun 26, 2017
View reviewed changes
src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory_Methods.cs
@@ -139,6 +139,9 @@ private static ConversionKind GetConversionKind(CSharp.ConversionKind kind)
case CSharp.ConversionKind.PointerToVoid:
return Semantics.ConversionKind.CSharp;

case CSharp.ConversionKind.InterpolatedString:
Copy link
Contributor

Choose a reason for hiding this comment

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

case CSharp.ConversionKind.InterpolatedString: [](start = 16, length = 46)

It feels like we should simply add this case to the previous switch section.

@AlekseyTs
Copy link
Contributor

Done with review pass.

AlekseyTs
AlekseyTs approved these changes Jun 26, 2017
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM

@333fred 333fred mentioned this pull request Jun 26, 2017
Closed
@333fred
Copy link
Member Author

333fred commented Jun 26, 2017

I'll hold off on submitting this until we decide if we actually want an InterpolatedString conversion kind.

@333fred 333fred added 4 - In Review A fix for the issue is submitted for review. Discussion Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation labels Jun 26, 2017
@CyrusNajmabadi
Copy link
Member

I'm ok with this going in as is. I think we'l want to do an overall review of conversions and conversion kinds for V1 soon though.

@333fred 333fred merged commit 3333bbc into dotnet:features/ioperation Jun 29, 2017
@333fred 333fred deleted the conversionkind.interpolatedstring branch June 29, 2017 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston left review comments

@jinujoseph jinujoseph jinujoseph approved these changes

@mavasani mavasani mavasani approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Area-Compilers cla-already-signed Concept-API This issue involves adding, removing, clarification, or modification of an API. Discussion Feature - IOperation IOperation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@333fred @cston @AlekseyTs @CyrusNajmabadi @mavasani @jinujoseph @dnfclas