From f47e918caaa49d6423564462c724dd0ff9dd577c Mon Sep 17 00:00:00 2001 From: Sean Hawkes Date: Sat, 18 Sep 2021 04:55:50 -0500 Subject: [PATCH 1/7] Moved program_options add_options to a helper function to allow defaults to be set by derived class constructor before immutable options are created by parent --- test/Common.cpp | 5 +++++ test/Common.h | 1 + test/tools/IsolTestOptions.cpp | 10 +++++++--- test/tools/IsolTestOptions.h | 1 + 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/test/Common.cpp b/test/Common.cpp index 9b2cfbb96..4be8e1625 100644 --- a/test/Common.cpp +++ b/test/Common.cpp @@ -91,6 +91,11 @@ CommonOptions::CommonOptions(std::string _caption): po::options_description::m_default_line_length, po::options_description::m_default_line_length - 23 ) +{ + +} + +void CommonOptions::addOptions() { options.add_options() ("evm-version", po::value(&evmVersionString), "which evm version to use") diff --git a/test/Common.h b/test/Common.h index c8a863d8c..4d97d720d 100644 --- a/test/Common.h +++ b/test/Common.h @@ -70,6 +70,7 @@ struct CommonOptions langutil::EVMVersion evmVersion() const; + virtual addOptions(); virtual bool parse(int argc, char const* const* argv); // Throws a ConfigException on error virtual void validate() const; diff --git a/test/tools/IsolTestOptions.cpp b/test/tools/IsolTestOptions.cpp index aaa087e47..487c7e8c7 100644 --- a/test/tools/IsolTestOptions.cpp +++ b/test/tools/IsolTestOptions.cpp @@ -58,6 +58,13 @@ std::string editorPath() IsolTestOptions::IsolTestOptions(std::string* _editor): CommonOptions(description) +{ + enforceViaYul = true; + enforceGasTest = (evmVersion() == langutil::EVMVersion{}); + enforceGasTestMinValue = 100000; +} + +void IsolTestOptions::addOptions() { options.add_options() ("editor", po::value(_editor)->default_value(editorPath()), "Path to editor for opening test files.") @@ -76,9 +83,6 @@ bool IsolTestOptions::parse(int _argc, char const* const* _argv) std::cout << options << std::endl; return false; } - enforceViaYul = true; - enforceGasTest = (evmVersion() == langutil::EVMVersion{}); - enforceGasTestMinValue = 100000; return res; } diff --git a/test/tools/IsolTestOptions.h b/test/tools/IsolTestOptions.h index aa9eb2564..7c1a5ebe1 100644 --- a/test/tools/IsolTestOptions.h +++ b/test/tools/IsolTestOptions.h @@ -35,6 +35,7 @@ struct IsolTestOptions: CommonOptions std::string testFilter = std::string{}; IsolTestOptions(std::string* _editor); + void addOptions() override; bool parse(int _argc, char const* const* _argv) override; void validate() const override; }; From ae7c617711cd8b93fa4d42b13791c2e8c5d4ca29 Mon Sep 17 00:00:00 2001 From: Sean Hawkes Date: Sat, 18 Sep 2021 05:43:09 -0500 Subject: [PATCH 2/7] Added call to addOptions virtual helper in CommonOptions::parse to add options from base/derived classes, modified interface of IsolTestOptions to include editor member variable set based on provided parameter in constructor as it is now needed by addOptions helper function --- test/Common.cpp | 1 + test/Common.h | 2 +- test/tools/IsolTestOptions.cpp | 1 + test/tools/IsolTestOptions.h | 3 ++- 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/Common.cpp b/test/Common.cpp index 4be8e1625..5a5a253ae 100644 --- a/test/Common.cpp +++ b/test/Common.cpp @@ -144,6 +144,7 @@ void CommonOptions::validate() const bool CommonOptions::parse(int argc, char const* const* argv) { po::variables_map arguments; + addOptions(); po::command_line_parser cmdLineParser(argc, argv); cmdLineParser.options(options); diff --git a/test/Common.h b/test/Common.h index 4d97d720d..f7c8fa733 100644 --- a/test/Common.h +++ b/test/Common.h @@ -70,7 +70,7 @@ struct CommonOptions langutil::EVMVersion evmVersion() const; - virtual addOptions(); + virtual void addOptions(); virtual bool parse(int argc, char const* const* argv); // Throws a ConfigException on error virtual void validate() const; diff --git a/test/tools/IsolTestOptions.cpp b/test/tools/IsolTestOptions.cpp index 487c7e8c7..bcbee4463 100644 --- a/test/tools/IsolTestOptions.cpp +++ b/test/tools/IsolTestOptions.cpp @@ -59,6 +59,7 @@ std::string editorPath() IsolTestOptions::IsolTestOptions(std::string* _editor): CommonOptions(description) { + editor = _editor; enforceViaYul = true; enforceGasTest = (evmVersion() == langutil::EVMVersion{}); enforceGasTestMinValue = 100000; diff --git a/test/tools/IsolTestOptions.h b/test/tools/IsolTestOptions.h index 7c1a5ebe1..4deb5f9aa 100644 --- a/test/tools/IsolTestOptions.h +++ b/test/tools/IsolTestOptions.h @@ -33,8 +33,9 @@ struct IsolTestOptions: CommonOptions bool noColor = false; bool acceptUpdates = false; std::string testFilter = std::string{}; + std::string editor = std::string{}; - IsolTestOptions(std::string* _editor); + explicit IsolTestOptions(const std::string* _editor); void addOptions() override; bool parse(int _argc, char const* const* _argv) override; void validate() const override; From 76fa00abedac5f1399a88635ac7b96ecaa0dec42 Mon Sep 17 00:00:00 2001 From: Sean Hawkes Date: Sat, 18 Sep 2021 06:22:27 -0500 Subject: [PATCH 3/7] Added invocation of base class addOptions in derived to populate list with common and derived options, fixed errors with editor member variable type mismatch --- test/tools/IsolTestOptions.cpp | 3 ++- test/tools/IsolTestOptions.h | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/test/tools/IsolTestOptions.cpp b/test/tools/IsolTestOptions.cpp index bcbee4463..44cf90655 100644 --- a/test/tools/IsolTestOptions.cpp +++ b/test/tools/IsolTestOptions.cpp @@ -67,8 +67,9 @@ IsolTestOptions::IsolTestOptions(std::string* _editor): void IsolTestOptions::addOptions() { + CommonOptions::addOptions(); options.add_options() - ("editor", po::value(_editor)->default_value(editorPath()), "Path to editor for opening test files.") + ("editor", po::value(editor)->default_value(editorPath()), "Path to editor for opening test files.") ("help", po::bool_switch(&showHelp), "Show this help screen.") ("no-color", po::bool_switch(&noColor), "Don't use colors.") ("accept-updates", po::bool_switch(&acceptUpdates), "Automatically accept expectation updates.") diff --git a/test/tools/IsolTestOptions.h b/test/tools/IsolTestOptions.h index 4deb5f9aa..f889f5b8f 100644 --- a/test/tools/IsolTestOptions.h +++ b/test/tools/IsolTestOptions.h @@ -33,9 +33,9 @@ struct IsolTestOptions: CommonOptions bool noColor = false; bool acceptUpdates = false; std::string testFilter = std::string{}; - std::string editor = std::string{}; + std::string* editor = nullptr; - explicit IsolTestOptions(const std::string* _editor); + explicit IsolTestOptions(std::string* _editor); void addOptions() override; bool parse(int _argc, char const* const* _argv) override; void validate() const override; From f2e59923ab4c632da6f12b16ddd62f50c6ec4578 Mon Sep 17 00:00:00 2001 From: Sean Hawkes Date: Sat, 18 Sep 2021 06:59:37 -0500 Subject: [PATCH 4/7] Added call to CommonOptions base class validate method to derived IsolTestOptions validate method to validate against both common and extended options --- test/tools/IsolTestOptions.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tools/IsolTestOptions.cpp b/test/tools/IsolTestOptions.cpp index 44cf90655..7e38b55b2 100644 --- a/test/tools/IsolTestOptions.cpp +++ b/test/tools/IsolTestOptions.cpp @@ -91,6 +91,7 @@ bool IsolTestOptions::parse(int _argc, char const* const* _argv) void IsolTestOptions::validate() const { + CommonOptions::validate(); static std::string filterString{"[a-zA-Z0-9_/*]*"}; static std::regex filterExpression{filterString}; assertThrow( From a875d1225ad10774b174919209812166e80dfa5c Mon Sep 17 00:00:00 2001 From: Sean Hawkes Date: Sat, 18 Sep 2021 07:13:04 -0500 Subject: [PATCH 5/7] Explicity set default values for program options based on initialized values from constructor --- test/Common.cpp | 24 ++++++++++++------------ test/tools/IsolTestOptions.cpp | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/test/Common.cpp b/test/Common.cpp index 5a5a253ae..b8593490e 100644 --- a/test/Common.cpp +++ b/test/Common.cpp @@ -95,23 +95,23 @@ CommonOptions::CommonOptions(std::string _caption): } -void CommonOptions::addOptions() +void CommonOptions::addOptions() { options.add_options() ("evm-version", po::value(&evmVersionString), "which evm version to use") ("testpath", po::value(&this->testPath)->default_value(solidity::test::testPath()), "path to test files") ("vm", po::value>(&vmPaths), "path to evmc library, can be supplied multiple times.") - ("ewasm", po::bool_switch(&ewasm), "tries to automatically find an ewasm vm and enable ewasm test-execution.") - ("no-semantic-tests", po::bool_switch(&disableSemanticTests), "disable semantic tests") - ("no-smt", po::bool_switch(&disableSMT), "disable SMT checker") - ("optimize", po::bool_switch(&optimize), "enables optimization") - ("enforce-via-yul", po::bool_switch(&enforceViaYul), "Enforce compiling all tests via yul to see if additional tests can be activated.") - ("enforce-compile-to-ewasm", po::bool_switch(&enforceCompileToEwasm), "Enforce compiling all tests to Ewasm to see if additional tests can be activated.") - ("enforce-gas-cost", po::bool_switch(&enforceGasTest), "Enforce checking gas cost in semantic tests.") - ("enforce-gas-cost-min-value", po::value(&enforceGasTestMinValue), "Threshold value to enforce adding gas checks to a test.") - ("abiencoderv1", po::bool_switch(&useABIEncoderV1), "enables abi encoder v1") - ("show-messages", po::bool_switch(&showMessages), "enables message output") - ("show-metadata", po::bool_switch(&showMetadata), "enables metadata output"); + ("ewasm", po::bool_switch(&ewasm)->default_value(ewasm), "tries to automatically find an ewasm vm and enable ewasm test-execution.") + ("no-semantic-tests", po::bool_switch(&disableSemanticTests)->default_value(disableSemanticTests), "disable semantic tests") + ("no-smt", po::bool_switch(&disableSMT)->default_value(disableSMT), "disable SMT checker") + ("optimize", po::bool_switch(&optimize)->default_value(optimize), "enables optimization") + ("enforce-via-yul", po::bool_switch(&enforceViaYul)->default_value(enforceViaYul), "Enforce compiling all tests via yul to see if additional tests can be activated.") + ("enforce-compile-to-ewasm", po::bool_switch(&enforceCompileToEwasm)->default_value(enforceCompileToEwasm), "Enforce compiling all tests to Ewasm to see if additional tests can be activated.") + ("enforce-gas-cost", po::bool_switch(&enforceGasTest)->default_value(enforceGasTest), "Enforce checking gas cost in semantic tests.") + ("enforce-gas-cost-min-value", po::value(&enforceGasTestMinValue)->default_value(enforceGasTestMinValue), "Threshold value to enforce adding gas checks to a test.") + ("abiencoderv1", po::bool_switch(&useABIEncoderV1)->default_value(useABIEncoderV1), "enables abi encoder v1") + ("show-messages", po::bool_switch(&showMessages)->default_value(showMessages), "enables message output") + ("show-metadata", po::bool_switch(&showMetadata)->default_value(showMetadata), "enables metadata output"); } void CommonOptions::validate() const diff --git a/test/tools/IsolTestOptions.cpp b/test/tools/IsolTestOptions.cpp index 7e38b55b2..3aecaeefa 100644 --- a/test/tools/IsolTestOptions.cpp +++ b/test/tools/IsolTestOptions.cpp @@ -70,9 +70,9 @@ void IsolTestOptions::addOptions() CommonOptions::addOptions(); options.add_options() ("editor", po::value(editor)->default_value(editorPath()), "Path to editor for opening test files.") - ("help", po::bool_switch(&showHelp), "Show this help screen.") - ("no-color", po::bool_switch(&noColor), "Don't use colors.") - ("accept-updates", po::bool_switch(&acceptUpdates), "Automatically accept expectation updates.") + ("help", po::bool_switch(&showHelp)->default_value(showHelp), "Show this help screen.") + ("no-color", po::bool_switch(&noColor)->default_value(noColor), "Don't use colors.") + ("accept-updates", po::bool_switch(&acceptUpdates)->default_value(acceptUpdates), "Automatically accept expectation updates.") ("test,t", po::value(&testFilter)->default_value("*/*"), "Filters which test units to include."); } From 5edabc014d34102827e78470acf96eecac67fb2a Mon Sep 17 00:00:00 2001 From: hawkess Date: Mon, 20 Sep 2021 13:17:35 -0500 Subject: [PATCH 6/7] Changed enforce-gas-cost and enforce-via-yul to accept explicit arguments --- test/Common.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Common.cpp b/test/Common.cpp index b8593490e..2a663aa1b 100644 --- a/test/Common.cpp +++ b/test/Common.cpp @@ -105,9 +105,9 @@ void CommonOptions::addOptions() ("no-semantic-tests", po::bool_switch(&disableSemanticTests)->default_value(disableSemanticTests), "disable semantic tests") ("no-smt", po::bool_switch(&disableSMT)->default_value(disableSMT), "disable SMT checker") ("optimize", po::bool_switch(&optimize)->default_value(optimize), "enables optimization") - ("enforce-via-yul", po::bool_switch(&enforceViaYul)->default_value(enforceViaYul), "Enforce compiling all tests via yul to see if additional tests can be activated.") + ("enforce-via-yul", po::value(&enforceViaYul)->default_value(enforceViaYul)->implicit_value(true), "Enforce compiling all tests via yul to see if additional tests can be activated.") ("enforce-compile-to-ewasm", po::bool_switch(&enforceCompileToEwasm)->default_value(enforceCompileToEwasm), "Enforce compiling all tests to Ewasm to see if additional tests can be activated.") - ("enforce-gas-cost", po::bool_switch(&enforceGasTest)->default_value(enforceGasTest), "Enforce checking gas cost in semantic tests.") + ("enforce-gas-cost", po::value(&enforceGasTest)->default_value(enforceGasTest)->implicit_value(true), "Enforce checking gas cost in semantic tests.") ("enforce-gas-cost-min-value", po::value(&enforceGasTestMinValue)->default_value(enforceGasTestMinValue), "Threshold value to enforce adding gas checks to a test.") ("abiencoderv1", po::bool_switch(&useABIEncoderV1)->default_value(useABIEncoderV1), "enables abi encoder v1") ("show-messages", po::bool_switch(&showMessages)->default_value(showMessages), "enables message output") From 4fd5093d9421c939c2895b3ec620127f99abceba Mon Sep 17 00:00:00 2001 From: hawkess Date: Mon, 20 Sep 2021 13:22:58 -0500 Subject: [PATCH 7/7] Removed pointer to external editor resource in IsolTestOptions, changed TestTool::handleResponse() to get editor value from m_options member variable --- test/tools/IsolTestOptions.cpp | 6 ++---- test/tools/IsolTestOptions.h | 4 ++-- test/tools/isoltest.cpp | 7 ++----- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/test/tools/IsolTestOptions.cpp b/test/tools/IsolTestOptions.cpp index 3aecaeefa..7f00e88f6 100644 --- a/test/tools/IsolTestOptions.cpp +++ b/test/tools/IsolTestOptions.cpp @@ -56,20 +56,18 @@ std::string editorPath() } -IsolTestOptions::IsolTestOptions(std::string* _editor): +IsolTestOptions::IsolTestOptions(): CommonOptions(description) { - editor = _editor; enforceViaYul = true; enforceGasTest = (evmVersion() == langutil::EVMVersion{}); - enforceGasTestMinValue = 100000; } void IsolTestOptions::addOptions() { CommonOptions::addOptions(); options.add_options() - ("editor", po::value(editor)->default_value(editorPath()), "Path to editor for opening test files.") + ("editor", po::value(&editor)->default_value(editorPath()), "Path to editor for opening test files.") ("help", po::bool_switch(&showHelp)->default_value(showHelp), "Show this help screen.") ("no-color", po::bool_switch(&noColor)->default_value(noColor), "Don't use colors.") ("accept-updates", po::bool_switch(&acceptUpdates)->default_value(acceptUpdates), "Automatically accept expectation updates.") diff --git a/test/tools/IsolTestOptions.h b/test/tools/IsolTestOptions.h index f889f5b8f..5bb02c2bc 100644 --- a/test/tools/IsolTestOptions.h +++ b/test/tools/IsolTestOptions.h @@ -33,9 +33,9 @@ struct IsolTestOptions: CommonOptions bool noColor = false; bool acceptUpdates = false; std::string testFilter = std::string{}; - std::string* editor = nullptr; + std::string editor = std::string{}; - explicit IsolTestOptions(std::string* _editor); + explicit IsolTestOptions(); void addOptions() override; bool parse(int _argc, char const* const* _argv) override; void validate() const override; diff --git a/test/tools/isoltest.cpp b/test/tools/isoltest.cpp index fa7b914aa..73adbc62b 100644 --- a/test/tools/isoltest.cpp +++ b/test/tools/isoltest.cpp @@ -121,8 +121,6 @@ public: fs::path const& _basepath, fs::path const& _path ); - - static string editor; private: enum class Request { @@ -145,7 +143,6 @@ private: static bool m_exitRequested; }; -string TestTool::editor; bool TestTool::m_exitRequested = false; TestTool::Result TestTool::process() @@ -258,7 +255,7 @@ TestTool::Request TestTool::handleResponse(bool _exception) } case 'e': cout << endl << endl; - if (system((TestTool::editor + " \"" + m_path.string() + "\"").c_str())) + if (system((m_options.editor + " \"" + m_path.string() + "\"").c_str())) cerr << "Error running editor command." << endl << endl; return Request::Rerun; case 'q': @@ -425,7 +422,7 @@ int main(int argc, char const *argv[]) setupTerminal(); { - auto options = std::make_unique(&TestTool::editor); + auto options = std::make_unique(); if (!options->parse(argc, argv)) return -1;