Implement Function Call Graph #10332
Conversation
|
There was an error when running Please check that your changes are working as intended. |
Marenz
force-pushed
the
function-call-graph
branch
from
9ed0514
to
8aa314d
Compare
| if (auto node = std::get_if<ASTNode const*>(&m_currentNode)) | ||
| callableNode = dynamic_cast<CallableDeclaration const*>(*node); | ||
|
|
||
| ContractDefinition const* super = |
There was a problem hiding this comment.
Please extract CompilerContext::superFunction to a common place instead of re-implementing it here.
Marenz
force-pushed
the
function-call-graph
branch
2 times, most recently
from
37d1e0d
to
3fff79e
Compare
Marenz
force-pushed
the
function-call-graph
branch
4 times, most recently
from
c923dd5
to
294f805
Compare
Marenz
force-pushed
the
function-call-graph
branch
from
294f805
to
3b541bf
Compare
|
In general: Are getters for public state variables intentionally exempt from both the call graph and its verification? |
|
I don't think getters are too important for now. The following components are planned to use the call graph and none needs support for getters:
|
| return m_graph->edges[_caller].insert(_callee).second; | ||
| } | ||
|
|
||
| void FunctionCallGraphBuilder::processFunction(CallableDeclaration const& _callable, bool _calledDirectly) |
There was a problem hiding this comment.
Maybe rename: functionReferenced to make it clear that this is not something like processFunctionCall
There was a problem hiding this comment.
Sure. I was planning to rename things to be clearer once I finish looking for failing cases anyway.
|
|
||
| for (auto arg: contract->baseContracts()) | ||
| arg->accept(*this); | ||
|
|
There was a problem hiding this comment.
Maybe we have to reset m_currentNode here?
There was a problem hiding this comment.
Yeah. Probably. I think this is what makes functions called from constructors have an edge from EntryCreation rather than from the constructor itself. I left it like that for now because it wasn't outright broken but I was planning to change that once I'm done with the fixes/tests.
| add(m_currentDispatch, &_callable); | ||
|
|
||
| if (!m_graph->edges.count(&_callable)) | ||
| visitCallable(&_callable); |
There was a problem hiding this comment.
I would feel safer if we just added this to a queue instead of visiting it right away.
There was a problem hiding this comment.
Good point. Right now it's recursive and we might run out of stack space on some very long call chains. I'll change that.
cameel
force-pushed
the
function-call-graph
branch
2 times, most recently
from
ee4c282
to
222f4bc
Compare
…) and use processCall() instead of visitCallable()
…ions, not ones that just access them
|
I went through all the comments and marked the ones that have been fixed as resolved. There are actually quite a few that are still relevant. I'll be fixing them now along with the rest of the stuff I mentioned in my previous comment, |
cameel
force-pushed
the
function-call-graph
branch
from
222f4bc
to
64582e0
Compare
|
This PR has become too big for it's own good and there seem to be problems with discussing some of the older comments. When I have another batch of fixes ready, I'm going to close it and just create a new one. |
|
I'm closing this PR in favor of #10973 which is now down and has fixes for nearly all stuff mentioned in the comments above. I didn't push any new code here to preserve it so that we can go back to older implementation and comments if needed. |
No description provided.