Skip to content

make InProcessEmitToolchain the default one #1093

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
Merged
merged 3 commits into from
Mar 7, 2019
Merged

make InProcessEmitToolchain the default one #1093

merged 3 commits into from
Mar 7, 2019

Conversation

adamsitnik
Copy link
Member

Today I saw some people using [InProcessAttribute] and I realized that it's still using the old one.

I think that we should switch by default to the new one.

ig-sinicyn
ig-sinicyn approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@ig-sinicyn ig-sinicyn left a comment

Choose a reason for hiding this comment

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

LGTM:)

@ig-sinicyn
Copy link
Contributor

@adamsitnik What to do with the old InProcessToolchain?

It has two codegen branches.

  • BenchmarkActionCodegen.ReflectionEmit that uses dynamic methods and is obsolete now.
  • BenchmarkActionCodegen.DelegateCombine that intended to work on platforms that do not support IL emitting.

Should I keep bot branches, remove the first one or remove the InProcessToolchain entirely?

@adamsitnik
Copy link
Member Author

@ig-sinicyn that is a very good question.

Maybe we should:

  • create new InProcessNoEmitToolchain based on BenchmarkActionCodegen.DelegateCombine
  • add an [Obsolete] comment to InProcessToolchain and explain that there is InProcessEmitToolchain and InProcessNoEmitToolchain which should be used instead? Make it a warning for one release and then switch to error after another release.

What do you think?

@adamsitnik adamsitnik merged commit 8968bbd into master Mar 7, 2019
@adamsitnik adamsitnik deleted the inProcessEmitDefault branch March 7, 2019 07:52
@adamsitnik adamsitnik added this to the v0.11.5 milestone Mar 7, 2019
@ig-sinicyn
Copy link
Contributor

Agreed:)
Will do the InProcessNoEmitToolchain on weekends.

@ig-sinicyn ig-sinicyn mentioned this pull request Mar 30, 2019
Merged
@SnakyBeaky SnakyBeaky mentioned this pull request Mar 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreyAkinshin AndreyAkinshin Awaiting requested review from AndreyAkinshin

1 more reviewer

@ig-sinicyn ig-sinicyn ig-sinicyn approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.11.5
Development

Successfully merging this pull request may close these issues.
2 participants
@adamsitnik @ig-sinicyn