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.

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

Compact format for AST-Json. #1810

Merged
merged 9 commits into from May 22, 2017

Conversation

Projects
None yet
4 participants
@chriseth
Contributor

chriseth commented Mar 20, 2017

@djujuu please take over from here. What is missing: Legacy-compatibility mode and several implementations of visit(...). The implementation of visit(SourceUnit) shows how children are added.

@chriseth

Comments until EnumDefinition.

Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
return true;
setJsonNode(_node, "StructDefinition", {
make_pair("name", _node.name()),
make_pair("visibility", visibility(_node.visibility())),

This comment has been minimized.

@chriseth

chriseth Mar 29, 2017

Contributor

We don't use visibility for struct definitions.

@chriseth

chriseth Mar 29, 2017

Contributor

We don't use visibility for struct definitions.

Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
}
void ASTJsonConverter::setJsonNode(
ASTNode const& _node,
string const& _nodeName,

This comment has been minimized.

@chriseth

chriseth Apr 10, 2017

Contributor

Please rename as below.

@chriseth

chriseth Apr 10, 2017

Contributor

Please rename as below.

This comment has been minimized.

@djudjuu

djudjuu Apr 12, 2017

Collaborator

I dont understand what you are reffering to here? Or is it obsolete now that we add the nodes in the correct order?

@djudjuu

djudjuu Apr 12, 2017

Collaborator

I dont understand what you are reffering to here? Or is it obsolete now that we add the nodes in the correct order?

Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
Show outdated Hide outdated libsolidity/ast/ASTJsonConverter.cpp
{
solAssert(!m_jsonNodePtrs.empty(), "Uneven json nodes stack. Internal error.");
m_jsonNodePtrs.pop();
return _node.id();

This comment has been minimized.

@chriseth

chriseth Apr 10, 2017

Contributor

Wouldn't it be better to return a Json::Value here?

@chriseth

chriseth Apr 10, 2017

Contributor

Wouldn't it be better to return a Json::Value here?

This comment has been minimized.

@djudjuu

djudjuu Apr 12, 2017

Collaborator

agreed.

@djudjuu

djudjuu Apr 12, 2017

Collaborator

agreed.

_node,
"SourceUnit",
{
make_pair("absolutePath", _node.annotation().path),

This comment has been minimized.

@axic

axic Apr 10, 2017

Member

Similarly to file below, this should either be file or the other one changed to match the name?

@axic

axic Apr 10, 2017

Member

Similarly to file below, this should either be file or the other one changed to match the name?

This comment has been minimized.

@chriseth

chriseth Apr 12, 2017

Contributor

This is different from the relative path that can be used in an import - it is the absolute path of the source unit itself (relative to the global root).

@chriseth

chriseth Apr 12, 2017

Contributor

This is different from the relative path that can be used in an import - it is the absolute path of the source unit itself (relative to the global root).

This comment has been minimized.

@axic

axic May 3, 2017

Member

Shouldn't it also include file key similar to ImportDirective? e.g. relative + absolute path.

@axic

axic May 3, 2017

Member

Shouldn't it also include file key similar to ImportDirective? e.g. relative + absolute path.

@djudjuu djudjuu added needs review and removed in progress labels Apr 12, 2017

@djudjuu djudjuu added needs review and removed needs review labels Apr 18, 2017

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira
Member

pirapira commented Apr 19, 2017

I'm seeing a failure from solfuzzer https://travis-ci.org/ethereum/solidity/jobs/223310983

@djudjuu djudjuu removed the needs review label Apr 19, 2017

@@ -2183,6 +2183,8 @@ string FunctionType::identifier() const
case Kind::ArrayPush: id += "arraypush"; break;
case Kind::ByteArrayPush: id += "bytearraypush"; break;
case Kind::ObjectCreation: id += "objectcreation"; break;
case Kind::Assert: id += "assert"; break;
case Kind::Require: id += "require";break;

This comment has been minimized.

@axic

axic Apr 20, 2017

Member

Oh, we've missed this.

@axic

axic Apr 20, 2017

Member

Oh, we've missed this.

This comment has been minimized.

@chriseth

chriseth Apr 24, 2017

Contributor

Please add a space.

@chriseth

chriseth Apr 24, 2017

Contributor

Please add a space.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 20, 2017

Member

This needs to be rebased and the StandardCompiler must also be adjusted for the ASTJsonConverter changes.

Member

axic commented Apr 20, 2017

This needs to be rebased and the StandardCompiler must also be adjusted for the ASTJsonConverter changes.

@djudjuu djudjuu added the in progress label Apr 21, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 21, 2017

Member

@djudjuu as discussed, for the AST import PR, the easy way would be to have a new test case in libsolidity/ASTJSON.cpp:

originalAst = <output form CompilerStack parser / ast>
originalJson = ASTJsonConverter().toJson(originalAst) 
newAst = ASTJsonImporter().fromJson(json)
assert(newAst == originalAst) // I guess there's no equality operator
newJson = ASTJsonConverter().toJson(newAst)
assert(newJson == originalJson) // I guess jsoncpp could have an equality operator
Member

axic commented Apr 21, 2017

@djudjuu as discussed, for the AST import PR, the easy way would be to have a new test case in libsolidity/ASTJSON.cpp:

originalAst = <output form CompilerStack parser / ast>
originalJson = ASTJsonConverter().toJson(originalAst) 
newAst = ASTJsonImporter().fromJson(json)
assert(newAst == originalAst) // I guess there's no equality operator
newJson = ASTJsonConverter().toJson(newAst)
assert(newJson == originalJson) // I guess jsoncpp could have an equality operator

@djudjuu djudjuu added needs review and removed in progress labels Apr 21, 2017

return true;
setJsonNode(_node, "Return", {
make_pair("expression", toJsonOrNull(_node.expression())),
make_pair("functionReturnParameters", idOrNull(_node.annotation().functionReturnParameters))

This comment has been minimized.

@axic

axic May 3, 2017

Member

functionReturnParameters or returnParameters?

@axic

axic May 3, 2017

Member

functionReturnParameters or returnParameters?

This comment has been minimized.

@chriseth

chriseth May 5, 2017

Contributor

I would stay with functionReturnParameters, otherwise it might be confused with returnValue.

@chriseth

chriseth May 5, 2017

Contributor

I would stay with functionReturnParameters, otherwise it might be confused with returnValue.

true);
return true;
std::vector<pair<string, Json::Value>> attributes = {
make_pair("prefix", _node.isPrefixOperation()),

This comment has been minimized.

@axic

axic May 3, 2017

Member

prefix os isPrefixOperation?

@axic

axic May 3, 2017

Member

prefix os isPrefixOperation?

}, true);
return true;
std::vector<pair<string, Json::Value>> attributes = {
make_pair(m_legacy ? "member_name" : "memberName", _node.memberName()),

This comment has been minimized.

@axic

axic May 3, 2017

Member

Also I guess we could make this name in the new mode?

@axic

axic May 3, 2017

Member

Also I guess we could make this name in the new mode?

This comment has been minimized.

@djudjuu

djudjuu May 3, 2017

Collaborator

sounds good to me

@djudjuu

djudjuu May 3, 2017

Collaborator

sounds good to me

@djudjuu djudjuu removed the needs review label May 17, 2017

@djudjuu djudjuu added needs review and removed needs review labels May 19, 2017

make_pair("constant", _node.isDeclaredConst())
}, true);
return true;
make_pair(m_legacy ? "constant" : "isDeclaredConst", _node.isDeclaredConst()),

This comment has been minimized.

@djudjuu

djudjuu May 19, 2017

Collaborator

i think this needs to be the other way around
m_legacy ? "isDeclaredConstant": "constant"....

edit:aah no..all good

@djudjuu

djudjuu May 19, 2017

Collaborator

i think this needs to be the other way around
m_legacy ? "isDeclaredConstant": "constant"....

edit:aah no..all good

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth May 19, 2017

Contributor

Just verified that this does not brake the tests for https://github.com/ethereum/browser-solidity/pull/508 (which makes extensive use of the legacy ast).

Contributor

chriseth commented May 19, 2017

Just verified that this does not brake the tests for https://github.com/ethereum/browser-solidity/pull/508 (which makes extensive use of the legacy ast).

@chriseth chriseth merged commit 8eead55 into develop May 22, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chriseth chriseth removed the needs review label May 22, 2017

@axic axic deleted the compactJson branch May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment