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.

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

[BREAKING] Disallow multiple events with same name and types. #9326

Merged
merged 1 commit into from
Jul 13, 2020
Merged

[BREAKING] Disallow multiple events with same name and types. #9326

merged 1 commit into from
Jul 13, 2020

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Jul 6, 2020

Fixes #3421

Is this how we wanted it?

@chriseth chriseth requested review from axic and ekpyron July 6, 2020 17:10
@chriseth chriseth force-pushed the eventOverwriting branch from 5a2210d to 2cc8f58 Compare July 6, 2020 17:11
@chriseth chriseth force-pushed the eventOverwriting branch from 2cc8f58 to ca60811 Compare July 6, 2020 17:14
chriseth
chriseth commented Jul 6, 2020
View reviewed changes
test/libsolidity/syntaxTests/events/inheritance_adds_anonymous.sol Outdated
event X() anonymous;
}
// ----
// DeclarationError 5883: (52-72): Event with same name and arguments defined twice.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this would also clash with event Y() anonymous, but I think we can ignore that.

Copy link
Member

Choose a reason for hiding this comment

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

Why would it clash with Y anonymous?

@axic axic changed the title Disallow multiple events with same name and types. [BREAKING] Disallow multiple events with same name and types. Jul 6, 2020
@hrkrshnn hrkrshnn force-pushed the eventOverwriting branch 2 times, most recently from cb8f68c to 6f052bb Compare July 7, 2020 11:09
hrkrshnn
hrkrshnn approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

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

LGTM. I rebased to fix the CI errors and fixed the two files with missing newline at EOF.

axic
axic reviewed Jul 7, 2020
View reviewed changes
test/libsolidity/syntaxTests/events/overloading_in_contract.sol
@@ -0,0 +1,5 @@
contract A {
event X();
event X(uint);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add another test case with indexed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean () and (uint indexed)? There is already one with (uint, uint) and (uint, uint indexed)

ekpyron
ekpyron reviewed Jul 7, 2020
View reviewed changes
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Maybe we should have a test for:

contract A { event X(uint); }
contract B is A {}
contract C is A {}
contract D is B, C {}

?
I think this should be allowed and I think it is allowed with this PR, but having a test might be good.

@chriseth chriseth force-pushed the eventOverwriting branch from 6f052bb to 50e5866 Compare July 8, 2020 16:15
@chriseth
Copy link
Contributor Author

chriseth commented Jul 8, 2020

@ekpyron added

@chriseth chriseth force-pushed the eventOverwriting branch from 50e5866 to e7dc993 Compare July 8, 2020 17:02
@chriseth chriseth merged commit 12f390a into breaking Jul 13, 2020
@chriseth chriseth deleted the eventOverwriting branch July 13, 2020 13:44
@chriseth chriseth mentioned this pull request Jul 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic left review comments

@hrkrshnn hrkrshnn hrkrshnn approved these changes

@ekpyron ekpyron ekpyron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chriseth @axic @ekpyron @hrkrshnn