-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Introduce universal compilation host to execution framework #15960
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
base: develop
Are you sure you want to change the base?
Conversation
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
08ce0db
to
9387714
Compare
6a47a21
to
9e50f8c
Compare
There was an error when running
Please check that your changes are working as intended. |
f79fbae
to
3b47946
Compare
9e5df30
to
eb50f43
Compare
eb50f43
to
132c6f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things need to be ironed out still, on a small scale you should make a pass over the PR and fix all the auto
s to use auto const&
and auto const*
where appropriate. Or respective mutable reference/pointer types. Or nothing, if you explicitly want to make a copy :)
Also (but optionally), I personally like to make as much const as possible. For ex., if you have a bool shouldDoSomething = m_compiler.shouldDoSomething()
will the sole purpose to pass it on and not modify it further, I declare it as bool const shouldDoSomething = ...
.
On the bigger scale of things, I am not a big fan of these optionals everywhere to capture experimental solidity in which some data members aren't available. It leads to a lot of bloat. Perhaps a variant with two clean distinct structs would be better. But not sure, really.
132c6f4
to
fe072fc
Compare
fe072fc
to
279ce36
Compare
279ce36
to
5826590
Compare
5826590
to
adb3578
Compare
Hey @clonker, I'm done reworking this. Would you mind having another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments :)
solAssert(m_sources.find(_sourceName) != m_sources.end(), "No source found."); | ||
auto const& source = m_sources.find(_sourceName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solAssert(m_sources.find(_sourceName) != m_sources.end(), "No source found."); | |
auto const& source = m_sources.find(_sourceName); | |
auto const& source = m_sources.find(_sourceName); | |
solAssert(source != m_sources.end(), "No source found."); |
m_compilerInput = CompilerInput{}; | ||
|
||
m_compilerInput.sourceCode = {{"", withPreamble(_sourceCode)}}; | ||
m_compilerInput.optimise = solidity::test::CommonOptions::get().optimize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_compilerInput.optimise = solidity::test::CommonOptions::get().optimize; | |
m_compilerInput.optimise = CommonOptions::get().optimize; |
auto object = contract->object; | ||
auto runtimeObject = contract->runtimeObject; | ||
auto assemblyItems = contract->assemblyItems.value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto object = contract->object; | |
auto runtimeObject = contract->runtimeObject; | |
auto assemblyItems = contract->assemblyItems.value(); | |
bytes const& object = contract->object; | |
bytes const& runtimeObject = contract->runtimeObject; | |
auto const& assemblyItems = contract->assemblyItems.value(); |
auto runtimeObject = contract->runtimeObject; | ||
auto assemblyItems = contract->assemblyItems.value(); | ||
|
||
PathGasMeter meter(assemblyItems, solidity::test::CommonOptions::get().evmVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathGasMeter meter(assemblyItems, solidity::test::CommonOptions::get().evmVersion()); | |
PathGasMeter meter(assemblyItems, CommonOptions::get().evmVersion()); |
soltestAssert(contract); | ||
soltestAssert(contract->runtimeAssemblyItems.has_value()); | ||
|
||
auto runtimeAssemblyItems = contract->runtimeAssemblyItems.value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto runtimeAssemblyItems = contract->runtimeAssemblyItems.value(); | |
auto const& runtimeAssemblyItems = contract->runtimeAssemblyItems.value(); |
runtimeAssemblyItems = std::make_optional(*items); | ||
} | ||
|
||
if (!m_stack.isExperimentalSolidity()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff can be simplified when experimental solidity is skipped
std::string errorInformation; | ||
for (auto const& error: m_stack.errors()) | ||
{ | ||
auto formatted = SourceReferenceFormatter::formatErrorInformation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd cram that directly into the errorInformation.append
. Then you're not prone of making another copy here. It's almost certainly being optimized away but yeah, not entirely certain either :)
|
||
#include <test/libsolidity/util/compiler/Compiler.h> | ||
|
||
#include <test/libsolidity/util/SoltestErrors.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused in the header
using Errors = ErrorList; | ||
|
||
/** | ||
* Abstraction of the internal compiler aka. `CompilerStack`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Abstraction of the internal compiler aka. `CompilerStack`. | |
* Abstraction of the `CompilerStack`. |
abstraction of the internal internal compiler stack :D
/// @param _input the compiler input with sources, options etc. optionally | ||
/// set. | ||
/// @returns the aggregated compiler output. | ||
CompilerOutput compile(CompilerInput const& _input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we were over this before, but why is this even stateful, especially since the whole stack gets reset each call to compile
?
Description
This PR implements a universal compiler host for running tests with various compilers (etc. the internal compiler aka.
CompilerStack
or standard JSON compiler).Details
It introduces some common types for the compiler inputs and its output. A compilation module that wraps the linked / internal compiler was added. It can be used in a newly added universal compiler host. This host is then used inside the test execution framework.
Outlook
The input / output types introduced by this PR were inspired by the
SolidityCompilationFramework
. In a future PR the universal compiler host could be used there as well.Ultimately, this PR makes it possible to implement a compilation module for the standard JSON compiler as well and use that in the pipeline.