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

Full inliner #3256

Merged
merged 11 commits into from May 7, 2018

Conversation

Projects
None yet
2 participants
@chriseth
Contributor

chriseth commented Nov 29, 2017

No description provided.

@chriseth chriseth changed the title from [WIP] Full inliner to Full inliner Jan 12, 2018

@chriseth chriseth requested a review from axic Jan 12, 2018

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jan 12, 2018

Contributor

@axic this is ready and has no dependencies.

Contributor

chriseth commented Jan 12, 2018

@axic this is ready and has no dependencies.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 2, 2018

Contributor

Added some fixes.

Contributor

chriseth commented Feb 2, 2018

Added some fixes.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 23, 2018

Member

@chriseth I think this is the next one to merge before the optimiser?

Member

axic commented Apr 23, 2018

@chriseth I think this is the next one to merge before the optimiser?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 24, 2018

Contributor

@axic yes, the last one.

Contributor

chriseth commented Apr 24, 2018

@axic yes, the last one.

Statement BodyCopier::operator()(VariableDeclaration const& _varDecl)
{
for (auto const& var: _varDecl.variables)
m_variableReplacements[var.name] = m_nameDispenser.newName(m_varNamePrefix + var.name);

This comment has been minimized.

@axic

axic May 2, 2018

Member

Either this should use the local wrapper newName or rather the wrapper should be removed.

@axic

axic May 2, 2018

Member

Either this should use the local wrapper newName or rather the wrapper should be removed.

This comment has been minimized.

@chriseth

chriseth May 2, 2018

Contributor

The wrapper is a function of InlineModifier, but here we are inside BodyCopier.

@chriseth

chriseth May 2, 2018

Contributor

The wrapper is a function of InlineModifier, but here we are inside BodyCopier.

This comment has been minimized.

@axic

axic May 4, 2018

Member

Sorry, sometimes it is confusing having all these different classes in the same file. Do we need the wrapper though?

@axic

axic May 4, 2018

Member

Sorry, sometimes it is confusing having all these different classes in the same file. Do we need the wrapper though?

This comment has been minimized.

@chriseth

chriseth May 4, 2018

Contributor

If you want, I can remove the wrapper also from the other class, I just found it more convenient.

@chriseth

chriseth May 4, 2018

Contributor

If you want, I can remove the wrapper also from the other class, I just found it more convenient.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth May 2, 2018

Contributor

Rebased.

Contributor

chriseth commented May 2, 2018

Rebased.

@chriseth chriseth merged commit d0bd549 into develop May 7, 2018

10 checks passed

ci/circleci: build_emscripten Your tests passed on CircleCI!
Details
ci/circleci: build_x86_linux Your tests passed on CircleCI!
Details
ci/circleci: build_x86_mac Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_external Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_solcjs Your tests passed on CircleCI!
Details
ci/circleci: test_x86_linux Your tests passed on CircleCI!
Details
ci/circleci: test_x86_mac Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axic axic deleted the fullInliner branch May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment