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] Simplify end visit for internal calls (refactor) #8952
Conversation
663d40f
to
daf82a3
a2db99a
to
bd6845d
75efd4f
to
9209810
bd6845d
to
4c847cc
|
I have changed the base branch to |
| else if (auto identifier = dynamic_cast<Identifier const*>(&_functionCall.expression())) | ||
| { | ||
| solAssert(!functionType->bound(), ""); | ||
| solUnimplementedAssert(!memberAccess || !functionType->bound(), ""); |
chriseth
May 19, 2020
Contributor
These assertions are all rather difficult to read. Would it be better to use something like
if (a)
solAssert(!b);
?
These assertions are all rather difficult to read. Would it be better to use something like
if (a)
solAssert(!b);
?
cameel
May 20, 2020
•
Author
Member
On the other hand I like that each one is now self-contained instead of having multiple nesting levels. That's a bit less of a problem now that I introduced referencedFunctionDeclaration() but before that I was getting a bit lost in it because the three main branches (identifier vs member vs something else) had overlapping but not exactly identical checks.
Also, they're just extra sanity checks, not a part of the logic and if they're not compact they obscure the more important bits of code in my opinion.
I agree that they're harder to read though. Before I go on with this change, here's a side-by side comparison with the best nested version I could come up with:
solUnimplementedAssert(!memberAccess || !functionType->bound(), "");
solAssert(memberAccess || !functionType->bound(), "");
solAssert(!memberAccess || !unresolvedFunctionDef || functionType->declaration() == *unresolvedFunctionDef, "");
solAssert(unresolvedFunctionDef || !functionType->hasDeclaration(), "");
// functionDef = ...
solAssert((functionDef == nullptr) == (unresolvedFunctionDef == nullptr), "");
solAssert(!functionDef || functionDef->isImplemented(), "");
if (memberAccess)
solUnimplementedAssert(!functionType->bound(), "");
else
solAssert(!functionType->bound(), "");
if (unresolvedFunctionDef && memberAccess)
solAssert(functionType->declaration() == *unresolvedFunctionDef, "");
else if (!unresolvedFunctionDef)
solAssert(!functionType->hasDeclaration(), "");
// functionDef = ...
if (functionDef)
{
solAssert(unresolvedFunctionDef, "");
solAssert(functionDef->isImplemented(), "");
}
else
solAssert(!unresolvedFunctionDef, "");
Should I change it?
On the other hand I like that each one is now self-contained instead of having multiple nesting levels. That's a bit less of a problem now that I introduced referencedFunctionDeclaration() but before that I was getting a bit lost in it because the three main branches (identifier vs member vs something else) had overlapping but not exactly identical checks.
Also, they're just extra sanity checks, not a part of the logic and if they're not compact they obscure the more important bits of code in my opinion.
I agree that they're harder to read though. Before I go on with this change, here's a side-by side comparison with the best nested version I could come up with:
solUnimplementedAssert(!memberAccess || !functionType->bound(), "");
solAssert(memberAccess || !functionType->bound(), "");
solAssert(!memberAccess || !unresolvedFunctionDef || functionType->declaration() == *unresolvedFunctionDef, "");
solAssert(unresolvedFunctionDef || !functionType->hasDeclaration(), "");
// functionDef = ...
solAssert((functionDef == nullptr) == (unresolvedFunctionDef == nullptr), "");
solAssert(!functionDef || functionDef->isImplemented(), "");if (memberAccess)
solUnimplementedAssert(!functionType->bound(), "");
else
solAssert(!functionType->bound(), "");
if (unresolvedFunctionDef && memberAccess)
solAssert(functionType->declaration() == *unresolvedFunctionDef, "");
else if (!unresolvedFunctionDef)
solAssert(!functionType->hasDeclaration(), "");
// functionDef = ...
if (functionDef)
{
solAssert(unresolvedFunctionDef, "");
solAssert(functionDef->isImplemented(), "");
}
else
solAssert(!unresolvedFunctionDef, "");Should I change it?
cameel
May 20, 2020
Author
Member
I changed it after all. The version with ifs is not that bad when it's not mixed with logic and the compact one is indeed harder to read. I just noticed that the condition for unresolvedFunctionDef && memberAccess should really have been just unresolvedFunctionDef.
I changed it after all. The version with ifs is not that bad when it's not mixed with logic and the compact one is indeed harder to read. I just noticed that the condition for unresolvedFunctionDef && memberAccess should really have been just unresolvedFunctionDef.
chriseth
May 20, 2020
Contributor
Not so sure about either :)
Not so sure about either :)
cameel
May 20, 2020
Author
Member
I tweaked it a bit more. Here's how it looks like now:
if (functionDef)
{
solAssert(memberAccess || identifier, "");
solAssert(functionType->declaration() == *functionDef, "");
if (identifier)
functionDef = &functionDef->resolveVirtual(m_context.mostDerivedContract());
solAssert(functionDef->isImplemented(), "");
}
else
solAssert(!functionType->hasDeclaration(), "");
if (memberAccess)
solUnimplementedAssert(!functionType->bound(), "");
else
solAssert(!functionType->bound(), "");
I tweaked it a bit more. Here's how it looks like now:
if (functionDef)
{
solAssert(memberAccess || identifier, "");
solAssert(functionType->declaration() == *functionDef, "");
if (identifier)
functionDef = &functionDef->resolveVirtual(m_context.mostDerivedContract());
solAssert(functionDef->isImplemented(), "");
}
else
solAssert(!functionType->hasDeclaration(), "");
if (memberAccess)
solUnimplementedAssert(!functionType->bound(), "");
else
solAssert(!functionType->bound(), "");| FunctionDefinition const* functionDef = (!unresolvedFunctionDef ? | ||
| nullptr : | ||
| (memberAccess ? | ||
| unresolvedFunctionDef : | ||
| &unresolvedFunctionDef->resolveVirtual(m_context.mostDerivedContract()) | ||
| ) | ||
| ); |
chriseth
May 19, 2020
Contributor
Isn't the following easier to understand? Yes, the variable is reassigned, but that's what we have a multi-paradigm language for, don't we? I would actually even go so far and use the same variable for unresolvedFunctionDef and functionDef. After all, what does "unresolved" mean in case we do not use a virtual function call?
Suggested change
FunctionDefinition const* functionDef = (!unresolvedFunctionDef ?
nullptr :
(memberAccess ?
unresolvedFunctionDef :
&unresolvedFunctionDef->resolveVirtual(m_context.mostDerivedContract())
)
);
FunctionDefinition const* functionDef = unresolvedFunctionDef;
if (dynamic_cast<Identifier const*>(&_functionCall.expression()))
functionDef = unresolvedFunctionDef->resolveVirtual(m_context.mostDerivedContract());
Isn't the following easier to understand? Yes, the variable is reassigned, but that's what we have a multi-paradigm language for, don't we? I would actually even go so far and use the same variable for unresolvedFunctionDef and functionDef. After all, what does "unresolved" mean in case we do not use a virtual function call?
| FunctionDefinition const* functionDef = (!unresolvedFunctionDef ? | |
| nullptr : | |
| (memberAccess ? | |
| unresolvedFunctionDef : | |
| &unresolvedFunctionDef->resolveVirtual(m_context.mostDerivedContract()) | |
| ) | |
| ); | |
| FunctionDefinition const* functionDef = unresolvedFunctionDef; | |
| if (dynamic_cast<Identifier const*>(&_functionCall.expression())) | |
| functionDef = unresolvedFunctionDef->resolveVirtual(m_context.mostDerivedContract()); |
chriseth
May 19, 2020
Contributor
Also note that you are implictly assuming here that "no member access" and "definition found" means that we have a direct identifier.
Also note that you are implictly assuming here that "no member access" and "definition found" means that we have a direct identifier.
cameel
May 20, 2020
Author
Member
Also note that you are implictly assuming here that "no member access" and "definition found" means that we have a direct identifier.
I've been hoping to keep casts to Identifer and MemberAccess hidden neatly inside referencedFunctionDeclaration(). Not very successfully. I'll add it back and add an assert for that case.
Isn't the following easier to understand? Yes, the variable is reassigned, but that's what we have a multi-paradigm language for, don't we?
ok, it's indeed simpler. I shouldn't have tried so hard to get rid of Identifier.
I would actually even go so far and use the same variable for unresolvedFunctionDef and functionDef. After all, what does "unresolved" mean in case we do not use a virtual function call?
In this particular case maybe reusing the variable is not that bad but in general I think it adds some unnecessary mental overhead to track what it refers to. Having separate names for things is better in my opinion. Of course it requires coming up with good names which can be hard at times.
Also note that you are implictly assuming here that "no member access" and "definition found" means that we have a direct identifier.
I've been hoping to keep casts to Identifer and MemberAccess hidden neatly inside referencedFunctionDeclaration(). Not very successfully. I'll add it back and add an assert for that case.
Isn't the following easier to understand? Yes, the variable is reassigned, but that's what we have a multi-paradigm language for, don't we?
ok, it's indeed simpler. I shouldn't have tried so hard to get rid of Identifier.
I would actually even go so far and use the same variable for
unresolvedFunctionDefandfunctionDef. After all, what does "unresolved" mean in case we do not use a virtual function call?
In this particular case maybe reusing the variable is not that bad but in general I think it adds some unnecessary mental overhead to track what it refers to. Having separate names for things is better in my opinion. Of course it requires coming up with good names which can be hard at times.
cameel
May 20, 2020
Author
Member
Done. I changed it according to your suggestions - added assertion for identifier, changed to reassignment and made the code reuse functionDef variable.
I still think that separate names would be clearer. Using the same name in assertions might give a false impression that they all still hold after the reassignment. This is not true for example for functionType->declaration() == *functionDef.
Done. I changed it according to your suggestions - added assertion for identifier, changed to reassignment and made the code reuse functionDef variable.
I still think that separate names would be clearer. Using the same name in assertions might give a false impression that they all still hold after the reassignment. This is not true for example for functionType->declaration() == *functionDef.
0d9a5eb
to
b7ee796
- Define IRHelpers::referencedFunctionDeclaration() to avoid repeating the same dynamic_casts over and over again.
b7ee796
to
0943333
Based on #8951 which needs to be merged first. Could also be rebased on #8949 which is immediately below but this requires resolving a minor conflict so I left them one on the other.It's based ondevelopnow.Refactoring changes extracted from #8943 (and before that from #8797). Does not affect functionality.
Related to #6788 and #8485.