Skip to content

Conversation

@chriseth
Copy link
Contributor

@chriseth chriseth commented Oct 14, 2021

Fixes #12090
Fixes #12149
Fixes #12158

@chriseth chriseth requested review from Marenz and cameel October 14, 2021 14:20
@chriseth chriseth force-pushed the nonamedlabelsifnotunique branch from 8ae7765 to f0c9a0d Compare October 14, 2021 14:34
Marenz
Marenz previously requested changes Oct 14, 2021
Copy link
Contributor

@Marenz Marenz left a comment

Choose a reason for hiding this comment

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

LGTM, just a stray space

@@ -0,0 +1 @@
--strict-assembly No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the annotations add much in this particular test. Might actually be more readable without them. Maybe it would be better to disable them?

Suggested change
--strict-assembly
--strict-assembly --debug-info none

@cameel
Copy link
Collaborator

cameel commented Oct 17, 2021

Does this PR fix also a similar case I found in inline assembly #12090 (comment)?

EDIT: Reported as a separate issue (#12158).

@@ -0,0 +1 @@
--experimental-via-ir --combined-json function-debug-runtime
Copy link
Collaborator

Choose a reason for hiding this comment

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

Output would be more readable if it was not all on a single line:

Suggested change
--experimental-via-ir --combined-json function-debug-runtime
--experimental-via-ir --combined-json function-debug-runtime --pretty-json --json-indent 4

Comment on lines 1 to 8
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}

Also in function_name_clash.sol below.

Comment on lines +1 to +8
contract C {
function f() public pure returns (uint r) {
assembly { function f() -> x { x := 1 } r := f() }
}
function g() public pure returns (uint r) {
assembly { function f() -> x { x := 2 } r := f() }
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this PR also fix #12158 or only #12149? If it solves #12158, I'd add the test case from there because it triggers a different assertion.

@chriseth
Copy link
Contributor Author

Incorporated #12149 into the test.

cameel
cameel previously approved these changes Oct 19, 2021
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

There are still some open comments that could be applied to improve the command-line test output but overall the fix seems correct and does the job even in the current form so I'm approving.

@cameel cameel dismissed Marenz’s stale review October 19, 2021 15:17

I think the comments were addressed.

@chriseth
Copy link
Contributor Author

Rebased, simplified commandline tests and updated the "fixes".

@chriseth chriseth merged commit e6e30f8 into develop Oct 25, 2021
@chriseth chriseth deleted the nonamedlabelsifnotunique branch October 25, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants