Properly explain all the analsys steps in CompilerStack#4327
Properly explain all the analsys steps in CompilerStack#4327
Conversation
|
@chriseth this is not ready to be merged, but please comment on lines which has question marks (some of them were just placeholders because we talked about them) |
| /// @returns the metadata JSON as a compact string for the given contract. | ||
| std::string createMetadata(Contract const& _contract) const; | ||
|
|
||
| /// @returns the computer source mapping string. |
libsolidity/ast/ASTVisitor.h
Outdated
| virtual void endVisitNode(ASTNode&) { } | ||
| }; | ||
|
|
||
| // Post visitor. endVisit ensure the child types are already visited |
There was a problem hiding this comment.
Why Post visitor, or did you meant to write Const visitor? I don't understand the second sentence.
| appendDelegatecallCheck(); | ||
|
|
||
| initializeContext(_contract, _contracts); | ||
| // This generate the dispatch function for externally visible functions |
| // and adds the function to the compilation (also internal functions, | ||
| // which are referenced directly or indirectly will be added to the same queue). | ||
| appendFunctionSelector(_contract); | ||
| // Compile functions listed in the above populated queue. |
There was a problem hiding this comment.
This processes the queue until it is empty.
| return false; | ||
|
|
||
| // This is the main name and type resolution loop. Needs to be run for every contract, because | ||
| // the special variables "this" and "super" must be set appropriately. |
There was a problem hiding this comment.
We could change that at some point and move this logic into the resolver.
| m_contracts[contract->fullyQualifiedName()].contract = contract; | ||
| } | ||
|
|
||
| // WHy cannot be this done in the above loop? |
There was a problem hiding this comment.
Simple answer: If we do it in the above loop, it is not possible to resolve the type of x in LibraryName.TypeName x; .
More detailed answer: It has been like that even before libraries (or cross-contract types) were introduced, so there needs to be another reason, but I cannot come up with an example right now. Perhaps it also has to do with inheritance.
|
|
||
| if (noErrors) | ||
| { | ||
| // Explain? |
There was a problem hiding this comment.
Checks that can only be done when all types of all AST nodes are known.
| shared_ptr<Compiler> compiler = make_shared<Compiler>(m_evmVersion, m_optimize, m_optimizeRuns); | ||
| compiledContract.compiler = compiler; | ||
|
|
||
| // create metadata |
There was a problem hiding this comment.
We should move the metadata stuff into its own function.
| } | ||
|
|
||
| /// Set the EVM version used before running compile. | ||
| /// When called without an argument it will revert to the default version (currently Byzantium). |
There was a problem hiding this comment.
I would not put the name into the comment, we will certainly forget to update it.
403d2e3 to
65a9332
Compare
26ec44b to
c2a26a1
Compare
|
Updated some of the comments. |
5494faf to
1aaf49e
Compare
|
@chriseth please review and merge this |
| if (!parseAndAnalyze()) | ||
| return false; | ||
|
|
||
| // Explain? |
There was a problem hiding this comment.
Ah, this one is missing.
Codecov Report
@@ Coverage Diff @@
## develop #4327 +/- ##
========================================
Coverage 87.92% 87.92%
========================================
Files 312 312
Lines 30958 30958
Branches 3623 3622 -1
========================================
Hits 27221 27221
Misses 2504 2504
Partials 1233 1233
Continue to review full report at Codecov.
|
|
|
||
| if (noErrors) | ||
| { | ||
| // Control flow graph: currently checks that variable is not used before it is assigned to. |
There was a problem hiding this comment.
But only for storage variables. Also I would not say what it currently does, because we almost certainly forget to update that, but rather that it for example does this.
There was a problem hiding this comment.
Can you give a proposed example text?
|
@chriseth please check the updated version |
No description provided.