Skip to content

Split fallback. #7385

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 1 commit into from
Nov 4, 2019
Merged

Split fallback. #7385

merged 1 commit into from
Nov 4, 2019

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented Sep 9, 2019 •
edited
Loading

Fixes #3198.

Main things missing:
What to do about solc-js and external tests? Update solc-js repo?
Recheck documentation.
Do we need more tests?

@ekpyron ekpyron force-pushed the fallbackSplit branch 4 times, most recently from 287c35b to 5908bf0 Compare September 11, 2019 11:31
@ethereum ethereum deleted a comment from stackenbotten Sep 11, 2019
@ekpyron ekpyron force-pushed the fallbackSplit branch 2 times, most recently from f48dc89 to beab816 Compare September 11, 2019 14:51
@erak erak mentioned this pull request Sep 13, 2019
Merged
@ekpyron ekpyron force-pushed the fallbackSplit branch 6 times, most recently from 94f88e5 to 96695eb Compare September 13, 2019 13:30
ekpyron
ekpyron commented Sep 13, 2019
View reviewed changes
test/cmdlineTests/standard_irOptimized_requested/output.json
@@ -33,6 +33,7 @@ object \"C_6\" {
}
default { }
}
if iszero(calldatasize()) { }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is optimized IR output - why isn't this removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look like optimized code in general. Maybe something is wrong with the optimizer there? The function fun_f_5 in the constructor, for example, should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok, the test is a bit confusing - it requests "irOptimized" via standard json, but does not actually enable yul optimization, so the code is in fact unoptimized... Should we run the yul optimizer by default, if "irOptimized" is requested? It's an issue separate from this PR in any case.

@ekpyron ekpyron force-pushed the fallbackSplit branch 2 times, most recently from ec0a501 to b61b57c Compare September 13, 2019 14:37
@ekpyron ekpyron marked this pull request as ready for review September 13, 2019 14:58
@ekpyron
Copy link
Member Author

ekpyron commented Sep 13, 2019

Taking this out of draft mode - it's not entirely ready, but I'd say it's getting close, and some review might be good.
Might be good if someone takes over for the rest, since I'll be off for a while.

Marenz
Marenz reviewed Sep 16, 2019
View reviewed changes
Marenz
Marenz reviewed Sep 16, 2019
View reviewed changes
Marenz
Marenz reviewed Sep 18, 2019
View reviewed changes
leonardoalt
leonardoalt reviewed Sep 19, 2019
View reviewed changes
leonardoalt
leonardoalt reviewed Sep 19, 2019
View reviewed changes
@leonardoalt
Copy link
Member

Rebased, needs review

@ekpyron
Copy link
Member Author

ekpyron commented Nov 4, 2019

Ok, still override related test failures - I'll fix.

@ekpyron ekpyron force-pushed the fallbackSplit branch 3 times, most recently from 32f1e7c to 49afeab Compare November 4, 2019 13:04
@ekpyron
Copy link
Member Author

ekpyron commented Nov 4, 2019

Hopefully good now.

chriseth
chriseth reviewed Nov 4, 2019
View reviewed changes
chriseth
chriseth reviewed Nov 4, 2019
View reviewed changes
chriseth
chriseth reviewed Nov 4, 2019
View reviewed changes
chriseth
chriseth reviewed Nov 4, 2019
View reviewed changes
test/libsolidity/syntaxTests/types/var_decl_val_mismatch.sol
@@ -13,5 +13,4 @@ contract n
}
// ----
// SyntaxError: (14-129): No visibility specified. Did you intend to add "external"?
// TypeError: (14-129): Fallback function must be defined as "external".
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this gone because we exit earlier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the order has changed due to the checks being moved from the contract level checker to the type checker (and the type checker exits early here).

chriseth
chriseth reviewed Nov 4, 2019
View reviewed changes
chriseth
chriseth reviewed Nov 4, 2019
View reviewed changes
chriseth
chriseth previously requested changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

Fine apart from the small comments.

chriseth
chriseth previously requested changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

Good to merge after squashing.

@chriseth chriseth merged commit cb743d6 into develop_060 Nov 4, 2019
@chriseth chriseth deleted the fallbackSplit branch November 4, 2019 16:38
@ekpyron ekpyron mentioned this pull request Nov 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Marenz Marenz Marenz left review comments

@leonardoalt leonardoalt leonardoalt left review comments

@chriseth chriseth chriseth approved these changes

@bshastry bshastry Awaiting requested review from bshastry

@christianparpart christianparpart Awaiting requested review from christianparpart

@erak erak Awaiting requested review from erak

Assignees
No one assigned
Labels
breaking change ⚠️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ekpyron @leonardoalt @Marenz @chriseth @erak