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

Remove LocalFunctionRewriting pass #21408

Merged
merged 2 commits into from
Aug 16, 2017

Conversation

agocke
Copy link
Member

@agocke agocke commented Aug 9, 2017

This PR depends on #21367

Perform synthesis of closure methods to an early pass before visitation,
meaning we no longer need to do a second visitation of the tree to lower
local functions.

Fixes https://github.com/dotnet/roslyn/projects/26#card-3753331

Perform synthesis of closure methods to an early pass before visitation,
meaning we no longer need to do a second visitation of the tree to lower
local functions.

Fixes https://github.com/dotnet/roslyn/projects/26#card-3753331
The baselines have been changed because we now do closure id and
synthessis in order of closure visitation, rather than bound node
visitation. Closure visitation visits all the closures in a given scope,
then recurs into nested scopes, while BoundNode visitation treats
closures as scopes themselves, so nested closures are visited before
closures declared in the same scope.
@agocke agocke force-pushed the remove-localfunctionreferencerewriter branch from 945e05c to 64090ae Compare August 11, 2017 03:10
@agocke
Copy link
Member Author

agocke commented Aug 11, 2017

ping @jaredpar @khyperia @VSadov @dotnet/roslyn-compiler for review

var start = loweredSymbol.ParameterCount - frameCount;
for (int i = start; i < loweredSymbol.ParameterCount; i++)
{
// will always be a LambdaFrame, it's always a capture frame
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert this?

Copy link
Member Author

Choose a reason for hiding this comment

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

3 lines down :)

MethodSymbol localFunc,
out BoundExpression receiver,
out MethodSymbol method,
ref ImmutableArray<BoundExpression> parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

This wasn't changed in this PR, but should this be arguments? (same with parametersBuilder, comments, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I'll file a follow up bug.

@gafter gafter self-requested a review August 15, 2017 18:24
Copy link
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

var syntax = originalMethod.DeclaringSyntaxReferences[0].GetSyntax();
var structClosures = closure.CapturedEnvironments
.Where(env => env.IsStruct).Select(env => env.SynthesizedEnvironment).AsImmutable();

Copy link
Member

Choose a reason for hiding this comment

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

Though this is beautiful, we tend to avoid Linq in the compilers.

Copy link
Member

Choose a reason for hiding this comment

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

(Yes, I know this is just moved existing code)


In reply to: 133294526 [](ancestors = 133294526)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'll change it.

translatedLambdaContainer = containerAsFrame = GetStaticFrame(Diagnostics, syntax);
closureKind = ClosureKind.Singleton;
closureOrdinal = LambdaDebugInfo.StaticClosureOrdinal;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can merge the tails of both branches (last statement is identical).

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@agocke agocke merged commit e887a8a into dotnet:master Aug 16, 2017
@agocke agocke deleted the remove-localfunctionreferencerewriter branch August 16, 2017 18:18
@agocke agocke mentioned this pull request Aug 17, 2017
333fred added a commit to 333fred/roslyn that referenced this pull request Aug 22, 2017
…-literal-text

* dotnet/features/ioperation:
  Fix no completion in partially written code before 'await'.
  parameter refness update in local functions/lambda should be caught a… (dotnet#21573)
  Don't offer to use pattern matching if a user defined operator was involved.
  Fix crash in VisualBasic.Binder.MemberLookup.AddLookupSymbolsInfoInTypeParameter when it is called with Cref TypeParameter. (dotnet#21586)
  Additional refactors.
  Refactor IConditionalChoiceExpression.
  use PerformIO utility
  put dispose under finally.
  Update to Microsoft.DiaSymReader.PortablePdb, Microsoft.DiaSymReader.Converter.Xml to 1.4.0-beta1-62016-01 (dotnet#21557)
  removed usage of ImmutableArray in json.net
  Update PULL_REQUEST_TEMPLATE.md
  Add test documentation to the PR template
  Remove LocalFunctionRewriting pass (dotnet#21408)
  PR feedbacks
  3 product changes and 1 test change
333fred added a commit to 333fred/roslyn that referenced this pull request Aug 22, 2017
…nversion

* dotnet/features/ioperation: (81 commits)
  Fix no completion in partially written code before 'await'.
  parameter refness update in local functions/lambda should be caught a… (dotnet#21573)
  Don't offer to use pattern matching if a user defined operator was involved.
  Fix crash in VisualBasic.Binder.MemberLookup.AddLookupSymbolsInfoInTypeParameter when it is called with Cref TypeParameter. (dotnet#21586)
  Additional refactors.
  Refactor variable names.
  Refactor IConditionalChoiceExpression.
  use PerformIO utility
  put dispose under finally.
  Update to Microsoft.DiaSymReader.PortablePdb, Microsoft.DiaSymReader.Converter.Xml to 1.4.0-beta1-62016-01 (dotnet#21557)
  removed usage of ImmutableArray in json.net
  Update PULL_REQUEST_TEMPLATE.md
  Add test documentation to the PR template
  Remove LocalFunctionRewriting pass (dotnet#21408)
  PR feedbacks
  Rename parameters.
  renamed methods.
  Rename method.
  Move integration test machines to 15.3 RTM (dotnet#21535)
  Update comment
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants