Skip to content

Add suggested stackalloc initializer tests#25656

Merged
jcouv merged 5 commits intodotnet:dev15.7.xfrom
alrz:stackalloc-init-tests
Mar 30, 2018
Merged

Add suggested stackalloc initializer tests#25656
jcouv merged 5 commits intodotnet:dev15.7.xfrom
alrz:stackalloc-init-tests

Conversation

@alrz
Copy link
Copy Markdown
Member

@alrz alrz commented Mar 21, 2018

Add tests from feature review (see #25160)

@alrz alrz requested a review from a team as a code owner March 21, 2018 20:36
@alrz
Copy link
Copy Markdown
Member Author

alrz commented Mar 21, 2018

/cc @VSadov for review.

@jcouv jcouv added Test Test failures in roslyn-CI and removed Auto-Merge If Tests Pass labels Mar 21, 2018
@jcouv jcouv added this to the 15.7 milestone Mar 21, 2018
var expressions = tree.GetCompilationUnitRoot().DescendantNodes().OfType<ImplicitStackAllocArrayCreationExpressionSyntax>().ToArray();
Assert.Equal(3, expressions.Length);

var @stackalloc = expressions[0];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@stackalloc [](start = 16, length = 11)

Nit: I'd recommend asserting the syntax, so it's unambiguous what we're looking at:
Assert.Equal("...", @stackalloc.ToString());

}", TestOptions.UnsafeReleaseDll).VerifyDiagnostics(
// (9,13): error CS8353: A result of a stackalloc expression of type 'Span<double>' cannot be used in this context because it may be exposed outside of the containing method
// _ = stackalloc double[2] { 1, 1.2 };
Diagnostic(ErrorCode.ERR_EscapeStackAlloc, "stackalloc double[2] { 1, 1.2 }").WithArguments("System.Span<double>").WithLocation(9, 13)
Copy link
Copy Markdown
Member

@jcouv jcouv Mar 29, 2018

Choose a reason for hiding this comment

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

I didn't understand this error. I would have expected this to work (nothing is escaping the scope of the method). Is there a bug with escape scope of discards?
@VSadov Can you confirm and file a bug as appropriate?

Copy link
Copy Markdown
Member Author

@alrz alrz Mar 29, 2018

Choose a reason for hiding this comment

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

looks like discard symbols are considered as returnable so we cant assign stackalloc with local lifetime.

Copy link
Copy Markdown
Member

@VSadov VSadov Mar 29, 2018

Choose a reason for hiding this comment

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

"Returnable" is much more common, so it makes sense to treat _ as returnable.
If they are considered not returnable, the cases like following would be errors:

return Method(out _ );  // what if Method returns its argument ?

Method(out _ , ref returnable);  // what if Method assigns one argument to another ?

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM to me with one question on discard scenario.
Thanks!

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 29, 2018

@VSadov for review (more tests for stackalloc init feature). Thanks

Copy link
Copy Markdown
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@VSadov
Copy link
Copy Markdown
Member

VSadov commented Mar 29, 2018

@dotnet-bot test windows_coreclr_release_prtest please

@jcouv jcouv merged commit 52a6eb3 into dotnet:dev15.7.x Mar 30, 2018
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 30, 2018

Merged. Thanks @alrz!

333fred added a commit to 333fred/roslyn that referenced this pull request Mar 30, 2018
* dotnet/dev15.7.x:
  Foreach to For refactoring (dotnet#25460)
  added more logging for completion to track some test failures where completion doesn't show up when it should have. (dotnet#25801)
  Add suggested stackalloc initializer tests (dotnet#25656)
  Addressed PR feedback.
  [SQLite] Fix continuous probing for sqlite
  Add missing file, update test for clearer output.
  Addressed PR feedback, updated ILocalFunctionOperation to final design specified in review.
  Added both ExpressionBody and BlockBody to the underlying BoundNode for local functions, and exposed both to IOperation consumers.
  Update PATH to our xcopy CLI
@alrz
Copy link
Copy Markdown
Member Author

alrz commented Mar 31, 2018

I don't know how this happened but TestUnmanaged_Span is failing on master (see #16808).
Did this even reach that branch?
Note: it's probably due to #24183
/cc @jcouv

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 31, 2018

@alrz Yes, I think it is due to merge of stackalloc initializers (from branch dev15.7.x) with #24183 in master branch.

PR for fix: #25854

@alrz alrz deleted the stackalloc-init-tests branch March 31, 2018 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Test Test failures in roslyn-CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants