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

[WIP] Import AST from JSON format (try 2) #2500

Open
wants to merge 10 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@djudjuu
Collaborator

djudjuu commented Jul 1, 2017

I rebased the branch, included the CR's from the original branch and cleaned up the code.

Replaces #2328.

@chriseth chriseth added the in progress label Jul 1, 2017

@djudjuu

This comment has been minimized.

Show comment
Hide comment
@djudjuu

djudjuu Jul 1, 2017

Collaborator

There is one bit about which i am unhappy and would like your help:
When the code compilation leads to warnings, the method SourceReferenceFormatter::printExceptionInformation() wants to output the sourceLocation, which will always print the entire Json-file. This is not really helpful.
Also, as InlineAssemblyeCode needs its own scanner, I needed to add an extra variable m_currentSourceName to ASTJsonImporter which is only used once.

I think the most appropriate way would be deal with the the generated warninsgs would be to output only the type of warning without the entire code. I am unsure about how to (elegantly) adjust the printExceptionInformation to that end.
i guess it's easiest to discuss this on a call...

Collaborator

djudjuu commented Jul 1, 2017

There is one bit about which i am unhappy and would like your help:
When the code compilation leads to warnings, the method SourceReferenceFormatter::printExceptionInformation() wants to output the sourceLocation, which will always print the entire Json-file. This is not really helpful.
Also, as InlineAssemblyeCode needs its own scanner, I needed to add an extra variable m_currentSourceName to ASTJsonImporter which is only used once.

I think the most appropriate way would be deal with the the generated warninsgs would be to output only the type of warning without the entire code. I am unsure about how to (elegantly) adjust the printExceptionInformation to that end.
i guess it's easiest to discuss this on a call...

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jul 1, 2017

Member

Is this replacing #2328?

Member

axic commented Jul 1, 2017

Is this replacing #2328?

@djudjuu

This comment has been minimized.

Show comment
Hide comment
@djudjuu

djudjuu Jul 1, 2017

Collaborator
Collaborator

djudjuu commented Jul 1, 2017

Show outdated Hide outdated Changelog.md
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jul 12, 2017

Member

@djudjuu pushed this changeset to the original branch to see which comments were left open (there are a couple), can you please check them there?

Member

axic commented Jul 12, 2017

@djudjuu pushed this changeset to the original branch to see which comments were left open (there are a couple), can you please check them there?

@axic axic changed the title from Import ast to [WIP] Import AST from JSON format (try 2) Aug 16, 2017

@djudjuu

This comment has been minimized.

Show comment
Hide comment
@djudjuu

djudjuu Sep 24, 2017

Collaborator

Hi @axic ,
I took another go at it, included all changes that were still missing, rebased it onto the current develop (in the process I had to squash my many commits into fewer and do a force push, although it meant that some of your comments are gone now, but otherwise i'd gone nuts. (They were some whitespace errors and some refactoring, no real design choices or the like....).
I also included the comments from #2328.

Also, I put a question in the comment in CompilerStack.cpp l. 846....it's in the metadata creation function, which is also the place where I felt the most unsure about my choices.

Collaborator

djudjuu commented Sep 24, 2017

Hi @axic ,
I took another go at it, included all changes that were still missing, rebased it onto the current develop (in the process I had to squash my many commits into fewer and do a force push, although it meant that some of your comments are gone now, but otherwise i'd gone nuts. (They were some whitespace errors and some refactoring, no real design choices or the like....).
I also included the comments from #2328.

Also, I put a question in the comment in CompilerStack.cpp l. 846....it's in the metadata creation function, which is also the place where I felt the most unsure about my choices.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Oct 10, 2017

Member

Sorry for the delay. Thanks! Pushed your version to #2328 and can confirm all the questions were addressed.

Member

axic commented Oct 10, 2017

Sorry for the delay. Thanks! Pushed your version to #2328 and can confirm all the questions were addressed.

djudjuu added some commits Oct 13, 2017

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 13, 2018

Contributor

cheer

Contributor

chriseth commented Mar 13, 2018

cheer

@djudjuu

This comment has been minimized.

Show comment
Hide comment
@djudjuu

djudjuu Mar 18, 2018

Collaborator

I (finally) updated the missing pieces, yet I don't know what to do about the appveyor error. @chriseth can you help?

Collaborator

djudjuu commented Mar 18, 2018

I (finally) updated the missing pieces, yet I don't know what to do about the appveyor error. @chriseth can you help?

@axic

Please rebase also.

@@ -73,6 +73,9 @@ class CommandLineInterface
/// Fills @a m_sourceCodes initially and @a m_redirects.
void readInputFilesAndConfigureRemappings();
/// Tries to read the ASTs from @ m_sourceCodes and to generate the input for
/// the compiler's importASTs()-function.
std::map<std::string, Json::Value const*> parseAstFromInput(); //use const here?

This comment has been minimized.

@axic

axic Mar 28, 2018

Member

Should use const if it isn't changing the state.

@axic

axic Mar 28, 2018

Member

Should use const if it isn't changing the state.

@@ -204,6 +204,7 @@ Bugfixes:
Features:
* Syntax Checker: Deprecated "throw" in favour of require(), assert() and revert().
* Type Checker: Warn if a local storage reference variable does not explicitly use the keyword ``storage``.
* AST: import a Json to AST.

This comment has been minimized.

@axic

axic Mar 28, 2018

Member

This is in the wrong version, also please capitalise to JSON.

@axic

axic Mar 28, 2018

Member

This is in the wrong version, also please capitalise to JSON.

/// @a _sourceList used to provide source names for the ASTs
ASTJsonImporter(std::map<std::string, Json::Value const*> _sourceList);
/// converts the

This comment has been minimized.

@axic

axic Mar 28, 2018

Member

Missing sentence?

@axic

axic Mar 28, 2018

Member

Missing sentence?

private:
template <typename T, typename... Args>
ASTPointer<T> createASTNode(Json::Value const& _node, Args&&... _args);
SourceLocation const createSourceLocation(Json::Value const& _node);

This comment has been minimized.

@axic

axic Mar 28, 2018

Member

Can this really be const?

@axic

axic Mar 28, 2018

Member

Can this really be const?

Literal::SubDenomination subdenomination(Json::Value const& _node);
Token::Value literalTokenKind(Json::Value const& _node);
Token::Value scanSingleToken(Json::Value _node);
std::map<std::string, Json::Value const*> m_sourceList;

This comment has been minimized.

@axic

axic Mar 28, 2018

Member

Can you separate the members from the helpers with a space?

@axic

axic Mar 28, 2018

Member

Can you separate the members from the helpers with a space?

template<class T>
ASTPointer<T> nullOrCast(Json::Value _json);
ASTPointer<ASTString> nullOrASTString(Json::Value _json, std::string const& _name);

This comment has been minimized.

@axic

axic Mar 28, 2018

Member

Can any of these be const? Can the _json parameter be a const&?

@axic

axic Mar 28, 2018

Member

Can any of these be const? Can the _json parameter be a const&?

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