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] Add new reserved keywords. #3627

Merged
merged 1 commit into from
Jun 12, 2018
Merged

[BREAKING] Add new reserved keywords. #3627

merged 1 commit into from
Jun 12, 2018

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented Mar 1, 2018 •
edited
Loading

Closes #2182.

@chriseth
Copy link
Contributor

chriseth commented Mar 1, 2018

If you add Closes #2182 to the description of the pull request, that issue will be automatically closed and also linked to this pull request.

chriseth
chriseth reviewed Mar 1, 2018
View reviewed changes
test/libsolidity/SolidityParser.cpp Outdated
};


for (const auto& keyword : keywords) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Coding style: no space in front of : except in a ? b : c.

Copy link
Contributor

@chriseth chriseth Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

And { on a line of its own.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for pointing that out :)

@chriseth chriseth changed the title Add new reserved keywords from #2182. [BREAKING] Add new reserved keywords from #2182. Mar 1, 2018
@chriseth
Copy link
Contributor

chriseth commented Mar 1, 2018

Please also add a line to Changelog.txt describing the change. It is probably a good idea for this PR to not add it to the changelog of 0.4.21 but instead start a new version 0.5.0. We will then merge it when 0.5.0 will be the next release.

@chriseth
Copy link
Contributor

chriseth commented Mar 1, 2018

Looks good! I'm not adding a positive review on purpose, so we do not accidentally merge this PR.

axic
axic reviewed Mar 2, 2018
View reviewed changes
test/libsolidity/SolidityParser.cpp Outdated
@@ -1164,6 +1164,56 @@ BOOST_AUTO_TEST_CASE(constant_is_keyword)
CHECK_PARSE_ERROR(text, "Expected identifier");
}

BOOST_AUTO_TEST_CASE(keyword_is_reserved)
Copy link
Member

Choose a reason for hiding this comment

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

Can this test case be extracted and merged with current keywords?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, see #3633

axic
axic previously requested changes Mar 2, 2018
View reviewed changes
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

There is a section in the documentation listing the reserved keywords, that should be updated too.

@ekpyron ekpyron force-pushed the additional-keywords branch 2 times, most recently from eb163bc to 1a5e5f0 Compare March 2, 2018 09:52
@ekpyron ekpyron mentioned this pull request Mar 2, 2018
Merged
@axic
Copy link
Member

axic commented Mar 2, 2018

Please rebase since #3633 was merged.

@chriseth
Copy link
Contributor

chriseth commented Mar 5, 2018

We should also add copy and copyof.

@axic
Copy link
Member

axic commented Mar 5, 2018

If you add Closes #2182 to the description of the pull request

Please do that and do not include it in the commit messages.

@ekpyron ekpyron self-assigned this May 4, 2018
@ekpyron ekpyron changed the base branch from develop to 050 May 8, 2018 09:47
@ekpyron ekpyron changed the title [BREAKING] Add new reserved keywords from #2182. [BREAKING] Add new reserved keywords. May 8, 2018
@ekpyron ekpyron force-pushed the additional-keywords branch 3 times, most recently from 1a680bf to 8e1e611 Compare May 8, 2018 09:50
This was referenced May 9, 2018
Closed
Closed
@ekpyron
Copy link
Member Author

ekpyron commented May 9, 2018

I removed the constructor keyword from this PR, since it breaks the tests unless the parser is updated, so it makes more sense to only add it in #4106.

@ekpyron
Copy link
Member Author

ekpyron commented May 15, 2018 •
edited
Loading

@chriseth I find the list of new keywords (and especially copy) to be a bit excessive myself and it seems to me more frequent breaking releases make more sense.
The question is whether there is a subset of this list that should definitely be in 0.5.0.

@chriseth
Copy link
Contributor

100%: constructor and emit
80%: mutable, immutable, unchecked, override
60%: alias, typedef

@axic axic changed the base branch from 050 to develop May 30, 2018 06:04
@axic
Copy link
Member

axic commented Jun 12, 2018

Rebased.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 12, 2018

@axic We still need to decide whether we want to keep this PR at all or which subset to keep.
constructor and emit are handled in separate commits, but we need to decide which of the other ones should in fact be reserved in 0.5.0 already.

@axic
Copy link
Member

axic commented Jun 12, 2018

I think we should just merge this. The only debatable one I think is topic and perhaps fallback because we don't know yet if we'll have wildcard or fallback, etc.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 12, 2018 •
edited
Loading

@axic I think what started the discussion actually was whether to keep copy, since it may be commonly used and break a lot of code. (@chriseth brought that up in an now outdated Changelog.md review comment)

@axic
Copy link
Member

axic commented Jun 12, 2018

Also if this is merged first and constructor/emit later then those are just moved from the reserved list to the "new keywords" line.

@axic
Copy link
Member

axic commented Jun 12, 2018

It seems copy is used in stringutils so that might be something we need to consider. Nothing else seems to break.

@axic
Copy link
Member

axic commented Jun 12, 2018

I'd take a shortcut and merge the non-contentious ones here and open a new PR for the rest.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 12, 2018

Removed copy, topic and fallback from the list.
I would still not include constructor and emit here, since the workarounds will fail once the keywords are introduced, so I think it's best to handle them separately altogether.

axic
axic previously approved these changes Jun 12, 2018
View reviewed changes
@axic
Copy link
Member

axic commented Jun 12, 2018

Confirmed against the issue.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 12, 2018

Removed const as well.

@axic axic merged commit d1e7e9e into develop Jun 12, 2018
@axic axic deleted the additional-keywords branch June 12, 2018 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth left review comments

@erak erak erak left review comments

@axic axic axic approved these changes

Assignees

@ekpyron ekpyron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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