Skip to content

Conversation

@erak
Copy link
Contributor

@erak erak commented Mar 21, 2018

Fixes #3567

Copy link
Contributor

@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.

Please adjusts (or add) tests for this.

@erak
Copy link
Contributor Author

erak commented Mar 23, 2018

@axic Added missing tests.

chriseth
chriseth previously approved these changes Mar 27, 2018
@chriseth
Copy link
Contributor

Looks good to me!

@erak erak self-assigned this Apr 3, 2018
Copy link
Contributor

@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.

Please check the resulting typeString (and typeIdentifier). It is a good idea to take all examples from the initial issue.

@erak
Copy link
Contributor Author

erak commented Apr 6, 2018

@axic I've realised that the tests used the legacy AST output instead of the new compact one. That's the reason why checks for typeName and typeString were missing. I've changed that...

Not sure about t_array$_t_array$_t_uint256_$dyn_storage_$dyn_storage_ptr for uint[][] memory rows;. It still looks wrong, but I wasn't sure how to fix the richIdentifier for ArrayType.

@chriseth What do you think about this remaing issue?

@chriseth
Copy link
Contributor

chriseth commented Apr 6, 2018

The rich identifier is fine, it is only important that it is really unique. They are not really exposed to the user as much as the other stuff is.

@erak erak force-pushed the astJsonTypeDescriptions branch from d6f3d38 to 5847ced Compare April 6, 2018 14:29
axic
axic previously requested changes Apr 9, 2018
Copy link
Contributor

@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.

Please have at least two cases, one which triggers _short = false and one which triggers _short = true.

@axic
Copy link
Contributor

axic commented Apr 9, 2018

Basically just extend all the other test cases with typeIdentifier expectation if they define a type.

@erak erak force-pushed the astJsonTypeDescriptions branch from 5847ced to 406a610 Compare April 9, 2018 11:35
@erak
Copy link
Contributor Author

erak commented Apr 9, 2018

@axic The JSON AST tests are using the legacy JSON output. That's why you won't find typeIdentifier but rather typeName and type. Actually those do not cover my changes since they only apply when using solc --ast-compact-json. I think changing the tests such that they use the new output could be done while deprecating the legacy output for 0.5.0.

I think I'd rather add some tests that cover non-shortened type pointer as you suggested.

@axic
Copy link
Contributor

axic commented Apr 9, 2018

The JSON AST tests are using the legacy JSON output.

In that case I think we should introduce two sub suites to group the legacy and the "compact" tests.

@erak
Copy link
Contributor Author

erak commented Apr 9, 2018

Great idea, will do that!

@axic
Copy link
Contributor

axic commented Apr 9, 2018

I actually meant that there can be multiple sub-levels of BOOST_AUTO_TEST_SUITE(<name>) within a single suite.

Also the case documentation contains both legacy and non-legacy tests, probably needs to be split into two.

@erak
Copy link
Contributor Author

erak commented Apr 9, 2018

@axic Ok, my idea was to split them into 2 files right away, in order to not end up with a very large one at some point. But if you prefer a single file with multiple sub-suites, I'll put it back. I don't have a stong opinion on that.

I've pulled out the legacy part of the documentation test. Did I miss something?

@axic
Copy link
Contributor

axic commented Apr 9, 2018

I don't have any preference, but if it is a split file I'd name it ASTLegacyJSON.

@axic axic closed this Apr 9, 2018
@axic axic reopened this Apr 9, 2018
@erak erak force-pushed the astJsonTypeDescriptions branch from ec72bc2 to 443e1b9 Compare April 9, 2018 15:55
@erak
Copy link
Contributor Author

erak commented Apr 9, 2018

@axic I've renamed the legacy file / suite and added two tests that cover long typeDescriptions for binary operations and identifiers.

@chriseth
Copy link
Contributor

Please add a changelog entry.

ekpyron
ekpyron previously approved these changes Apr 12, 2018
Copy link
Collaborator

@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.

Looks fine to me now!

@ekpyron ekpyron force-pushed the astJsonTypeDescriptions branch from ad3fcb3 to b2b5775 Compare April 12, 2018 09:19
@ekpyron
Copy link
Collaborator

ekpyron commented Apr 12, 2018

Rebased.

Changelog.md Outdated
* General: Limit the number of errors output in a single run to 256.
* General: Support accessing dynamic return data in post-byzantium EVMs.
* Interfaces: Allow overriding external functions in interfaces with public in an implementing contract.
* JSON AST: Use short string representation for TypePointers.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a bugfix, and perhaps also phrased differently for the changelog: Remove storage qualifier for type name strings.

@ekpyron ekpyron force-pushed the astJsonTypeDescriptions branch from b2b5775 to 5ffe3d8 Compare April 12, 2018 09:57
@chriseth
Copy link
Contributor

Sorry, please rebase.

@ekpyron ekpyron force-pushed the astJsonTypeDescriptions branch from 5ffe3d8 to 4bd31aa Compare April 12, 2018 19:15
@ekpyron ekpyron dismissed axic’s stale review April 13, 2018 08:53

Should be fine now.

@chriseth chriseth merged commit 2001cc6 into develop Apr 13, 2018
@axic axic deleted the astJsonTypeDescriptions branch April 13, 2018 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants