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.

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

Added tests for Out Variable Declarations in new expression contexts. #11987

Merged
merged 1 commit into from
Jun 15, 2016

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review.

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review.

@"
public class X
{
public static void Main()
Copy link
Member

Choose a reason for hiding this comment

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

Remove Main and commented out code below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to keep the commented code in the test source as placeholders for where we would wan't to test let declarations. This is consistent with pattern matching tests.

@jcouv
Copy link
Member

jcouv commented Jun 14, 2016

LGTM. Thanks

{
throw new System.InvalidOperationException();
}
catch (System.Exception e) when (Dummy(TakeOutParam(e, out var x1), x1))
Copy link
Member

Choose a reason for hiding this comment

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

Would it be interesting to capture x1 inside when - like Dummy(TakeOutParam(e, out var x1), ()=>x1) ?

Copy link
Member

@VSadov VSadov Jun 14, 2016

Choose a reason for hiding this comment

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

Another thing to try is to add await in the catch/finally.
Such try's need to be significantly rewritten and I am not sure that rewriter has ever seen locals declared in "when".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are good suggestions. In this particular case it is not likely to run into problems because exception variable is sharing the scope and we should be able to capture it. I added a work item into #11566 to make sure we have a good coverage for lambdas and await.

@VSadov
Copy link
Member

VSadov commented Jun 14, 2016

LGTM

void Test1()
{
try {}
catch when (TakeOutParam(out var x1) && x1 > 0)
Copy link
Member

Choose a reason for hiding this comment

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

The tests as written are extremely difficult to review because of the large number of mixed positive and negative tests aggregated together. I cannot easily see both the test and the verification code on the same screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize for this inconvenience. At the moment, I am simply porting tests that we have for pattern variables. The base line comes from there too, modulo the source line content and the location on the line. I will take this feedback into consideration for non-ported tests, I heard the same feedback from other team members too.

@AlekseyTs AlekseyTs merged commit f1df00d into dotnet:features/outvar Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants