Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
[Sol->Yul] Split internal dispatch into separate enumeration and code generation (refactor) #8951
Conversation
fb815ad
to
98b9955
9bb8922
to
08d1984
15b0283
to
75efd4f
97378d0
to
fc85050
9209810
to
8eb6838
| @@ -102,7 +102,8 @@ class IRGenerationContext | |||
|
|
|||
| std::string newYulVariable(); | |||
|
|
|||
| std::string internalDispatch(YulArity const& _arity); | |||
| std::string generateInternalDispatchFunction(YulArity const& _arity); | |||
| std::string generateInternalDispatchFunction(YulArity const& _arity, std::set<FunctionDefinition const*> _functions); | |||
chriseth
May 19, 2020
Contributor
I still don't like this split very much. The problem here is that both functions are named the same and calling one function at whatever time is always safe but calling the second function with the wrong second argument can result in severe damage. Is it compatible with what follows to do it as follows?
string IRGenerationContext::generateInternalDispatchFunction(YulArity const& _arity)
{
string funName = IRNames::internalDispatch(_arity);
set<FunctionDefinition const*> functions = collectFunctionsOfArity(_arity); // this is the second function
return m_functions.createFunction(funName, [funName, _arity, functions(move(_functions))]() {
...
});
}
I still don't like this split very much. The problem here is that both functions are named the same and calling one function at whatever time is always safe but calling the second function with the wrong second argument can result in severe damage. Is it compatible with what follows to do it as follows?
string IRGenerationContext::generateInternalDispatchFunction(YulArity const& _arity)
{
string funName = IRNames::internalDispatch(_arity);
set<FunctionDefinition const*> functions = collectFunctionsOfArity(_arity); // this is the second function
return m_functions.createFunction(funName, [funName, _arity, functions(move(_functions))]() {
...
});
}
cameel
May 20, 2020
Author
Member
Done.
It's fine for this PR but it forced me to reorganize the code in #8797 in an awkward way. I had to move the whole generateInternalDispatch() from IRGenerator to IRGenerationContext because otherwise I'd have to add more methods to context only to allow it to get the arities without consuming the dispatch content. It doesn't feel quite right to have a whole part of code generation (rather than just helpers) in the context.
Done.
It's fine for this PR but it forced me to reorganize the code in #8797 in an awkward way. I had to move the whole generateInternalDispatch() from IRGenerator to IRGenerationContext because otherwise I'd have to add more methods to context only to allow it to get the arities without consuming the dispatch content. It doesn't feel quite right to have a whole part of code generation (rather than just helpers) in the context.
8eb6838
to
2094b09
…ons from generateInternalDispatch() into a separate function
2094b09
to
6c6a8a7
Based on #8949 which needs to be merged first.
Refactoring changes extracted from #8943 (and before that from #8797). Does not affect functionality.
Related to #6788 and #8485.