From 00bdff104379dd97d5af99cecf34133dd77fb5ba Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 27 Jul 2020 18:37:19 +0100 Subject: [PATCH 1/8] Make `goto_cc_modet::help` non-virtual Making this member function non-virtual makes it easier to follow the control flow because it is clear it not overridden in any sub class. This is should be easy enough to add back should a need to override this arise. --- src/goto-cc/goto_cc_mode.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/goto-cc/goto_cc_mode.h b/src/goto-cc/goto_cc_mode.h index 6dbad84d3ce..5853e51c396 100644 --- a/src/goto-cc/goto_cc_mode.h +++ b/src/goto-cc/goto_cc_mode.h @@ -24,7 +24,7 @@ class goto_cc_modet:public messaget virtual int main(int argc, const char **argv); virtual int doit()=0; virtual void help_mode()=0; - virtual void help(); + void help(); virtual void usage_error(); goto_cc_modet( From 1fc57a685a89bb99559c77c852e72de07cc9f1cc Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 27 Jul 2020 20:03:34 +0100 Subject: [PATCH 2/8] Move `goto_cc_cmdlinet::have_infile_arg` to `.cpp` Putting implementations in `.cpp` files is the best-practise because it reduces compile times. --- src/goto-cc/goto_cc_cmdline.cpp | 10 ++++++++++ src/goto-cc/goto_cc_cmdline.h | 9 +-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/goto-cc/goto_cc_cmdline.cpp b/src/goto-cc/goto_cc_cmdline.cpp index 52fd45b5d7e..f0879099bd9 100644 --- a/src/goto-cc/goto_cc_cmdline.cpp +++ b/src/goto-cc/goto_cc_cmdline.cpp @@ -134,3 +134,13 @@ void goto_cc_cmdlinet::add_infile_arg(const std::string &arg) fclose(tmp); } } + +bool goto_cc_cmdlinet::have_infile_arg() const +{ + for(parsed_argvt::const_iterator it = parsed_argv.begin(); + it != parsed_argv.end(); + it++) + if(it->is_infile_name) + return true; + return false; +} diff --git a/src/goto-cc/goto_cc_cmdline.h b/src/goto-cc/goto_cc_cmdline.h index 65faa1d413b..ab56d081e55 100644 --- a/src/goto-cc/goto_cc_cmdline.h +++ b/src/goto-cc/goto_cc_cmdline.h @@ -68,14 +68,7 @@ class goto_cc_cmdlinet:public cmdlinet typedef std::list parsed_argvt; parsed_argvt parsed_argv; - bool have_infile_arg() const - { - for(parsed_argvt::const_iterator - it=parsed_argv.begin(); it!=parsed_argv.end(); it++) - if(it->is_infile_name) - return true; - return false; - } + bool have_infile_arg() const; std::string stdin_file; From f0b4f89536e7ac77c113e88461b9f8fa4f970081 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 27 Jul 2020 20:08:27 +0100 Subject: [PATCH 3/8] Refactor `goto_cc_cmdlinet::have_infile_arg` using `std::any_of` This avoids the need to declare and mutate iterators. Its states what it is doing by means of the name of the algorithm - `std::any_of`, which saves on reading a loop to work it out. --- src/goto-cc/goto_cc_cmdline.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/goto-cc/goto_cc_cmdline.cpp b/src/goto-cc/goto_cc_cmdline.cpp index f0879099bd9..a70c4bf7575 100644 --- a/src/goto-cc/goto_cc_cmdline.cpp +++ b/src/goto-cc/goto_cc_cmdline.cpp @@ -13,10 +13,11 @@ Date: April 2010 #include "goto_cc_cmdline.h" -#include +#include #include -#include #include +#include +#include #include #include @@ -137,10 +138,8 @@ void goto_cc_cmdlinet::add_infile_arg(const std::string &arg) bool goto_cc_cmdlinet::have_infile_arg() const { - for(parsed_argvt::const_iterator it = parsed_argv.begin(); - it != parsed_argv.end(); - it++) - if(it->is_infile_name) - return true; - return false; + return std::any_of( + parsed_argv.cbegin(), parsed_argv.cend(), [](const argt &arg) { + return arg.is_infile_name; + }); } From 1eaa2602303c9bc6ce9fd288c3f0090b4574c58e Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Tue, 4 Aug 2020 16:12:13 +0100 Subject: [PATCH 4/8] Make `goto_cc_modet::main` non-virtual Making this member function non-virtual makes it easier to follow the control flow because it is clear it not overridden in any sub class. This is should be easy enough to add back should a need to override this arise. --- src/goto-cc/goto_cc_mode.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/goto-cc/goto_cc_mode.h b/src/goto-cc/goto_cc_mode.h index 5853e51c396..1bf2b9dd6b7 100644 --- a/src/goto-cc/goto_cc_mode.h +++ b/src/goto-cc/goto_cc_mode.h @@ -21,7 +21,7 @@ Date: June 2006 class goto_cc_modet:public messaget { public: - virtual int main(int argc, const char **argv); + int main(int argc, const char **argv); virtual int doit()=0; virtual void help_mode()=0; void help(); From 03135587340fbf2613b5d34265e955c684a053df Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 27 Jul 2020 19:13:20 +0100 Subject: [PATCH 5/8] Move `prefix_in_list` to the single location where it is used This makes the `goto_cc_cmdlinet` class easier to follow because it can be read without seeing this special case for `armcc`. --- src/goto-cc/armcc_cmdline.cpp | 14 ++++++++++++++ src/goto-cc/goto_cc_cmdline.cpp | 17 ----------------- src/goto-cc/goto_cc_cmdline.h | 5 ----- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/goto-cc/armcc_cmdline.cpp b/src/goto-cc/armcc_cmdline.cpp index d0a16ac95a0..5b0607e3edc 100644 --- a/src/goto-cc/armcc_cmdline.cpp +++ b/src/goto-cc/armcc_cmdline.cpp @@ -265,6 +265,20 @@ static const char *options_with_arg[]= nullptr }; +bool prefix_in_list(const char *option, const char **list, std::string &prefix) +{ + for(std::size_t i = 0; list[i] != nullptr; i++) + { + if(strncmp(option, list[i], strlen(list[i])) == 0) + { + prefix = std::string(list[i]); + return true; + } + } + + return false; +} + bool armcc_cmdlinet::parse(int argc, const char **argv) { for(int i=1; i optnr; diff --git a/src/goto-cc/goto_cc_cmdline.h b/src/goto-cc/goto_cc_cmdline.h index ab56d081e55..e446de32352 100644 --- a/src/goto-cc/goto_cc_cmdline.h +++ b/src/goto-cc/goto_cc_cmdline.h @@ -26,11 +26,6 @@ class goto_cc_cmdlinet:public cmdlinet static bool in_list(const char *option, const char **list); - static bool prefix_in_list( - const char *option, - const char **list, - std::string &prefix); - // never fails, will add if not found std::size_t get_optnr(const std::string &option); From cc12dff8223a9cddb579f48c80d8982c14f9fb27 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 27 Jul 2020 19:28:18 +0100 Subject: [PATCH 6/8] Refactor `prefix_in_list` to return its result using `optionalt` This avoids using an output parameter, which makes it more straight forward to read the data flow. --- src/goto-cc/armcc_cmdline.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/goto-cc/armcc_cmdline.cpp b/src/goto-cc/armcc_cmdline.cpp index 5b0607e3edc..7a63af6b528 100644 --- a/src/goto-cc/armcc_cmdline.cpp +++ b/src/goto-cc/armcc_cmdline.cpp @@ -11,6 +11,8 @@ Author: Daniel Kroening #include "armcc_cmdline.h" +#include + #include #include @@ -265,18 +267,17 @@ static const char *options_with_arg[]= nullptr }; -bool prefix_in_list(const char *option, const char **list, std::string &prefix) +optionalt prefix_in_list(const char *option, const char **list) { for(std::size_t i = 0; list[i] != nullptr; i++) { if(strncmp(option, list[i], strlen(list[i])) == 0) { - prefix = std::string(list[i]); - return true; + return {list[i]}; } } - return false; + return {}; } bool armcc_cmdlinet::parse(int argc, const char **argv) @@ -291,35 +292,34 @@ bool armcc_cmdlinet::parse(int argc, const char **argv) } // it starts with - and it isn't "-" - - std::string prefix; + optionalt prefix; if(in_list(argv[i], options_no_arg)) { // options that don't have any arguments set(argv[i]); } - else if(prefix_in_list(argv[i], options_with_arg, prefix)) + else if((prefix = prefix_in_list(argv[i], options_with_arg))) { // options that have a separated _or_ concatenated argument - if(strlen(argv[i])>prefix.size()) // concatenated? - set(prefix, std::string(argv[i], prefix.size(), std::string::npos)); + if(strlen(argv[i]) > prefix->size()) // Concatenated. + set(*prefix, std::string(argv[i], prefix->size(), std::string::npos)); else { // Separated. if(i!=argc-1) // Guard against end of command line. { - set(prefix, argv[i+1]); + set(*prefix, argv[i + 1]); i++; } else - set(prefix, ""); + set(*prefix, ""); } } - else if(prefix_in_list(argv[i], options_with_prefix, prefix)) + else if((prefix = prefix_in_list(argv[i], options_with_prefix))) { // options that have a concatenated argument - set(prefix, std::string(argv[i], prefix.size(), std::string::npos)); + set(*prefix, std::string(argv[i], prefix->size(), std::string::npos)); } else { // unrecognized option From eb7b03216df891e7824e0c9068c4d8a6e064ee34 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Tue, 4 Aug 2020 17:38:30 +0100 Subject: [PATCH 7/8] Refactor `prefix_in_list` to take vector of strings Because this gives us compatability with modern C++ features such as ranged for loops and the algorithms in the standard template library. Because using a standard container rather than a null terminated array improves memory safety. Clang format is off for the lists of options in order to keep the existing manual formatting. This existing formatting is consistent with the other lists of options. --- src/goto-cc/armcc_cmdline.cpp | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/goto-cc/armcc_cmdline.cpp b/src/goto-cc/armcc_cmdline.cpp index 7a63af6b528..be1d2c19c67 100644 --- a/src/goto-cc/armcc_cmdline.cpp +++ b/src/goto-cc/armcc_cmdline.cpp @@ -12,9 +12,13 @@ Author: Daniel Kroening #include "armcc_cmdline.h" #include +#include +#include #include #include +#include +#include /// parses the command line options into a cmdlinet /// \par parameters: argument count, argument strings @@ -197,7 +201,8 @@ static const char *options_no_arg[]= nullptr }; -static const char *options_with_prefix[]= +// clang-format off +static const std::vector options_with_prefix { "--project=", "--workdir=", @@ -243,11 +248,10 @@ static const char *options_with_prefix[]= "--configure_sysroot=", "--configure_cpp_headers=", "--configure_extra_includes=", - "--configure_extra_libraries=", - nullptr + "--configure_extra_libraries=" }; -static const char *options_with_arg[]= +static const std::vector options_with_arg { // goto-cc specific "--verbosity", @@ -263,21 +267,20 @@ static const char *options_with_arg[]= "-Warmcc,", "-o", "--cpu", - "--apcs", - nullptr + "--apcs" }; +// clang-format on -optionalt prefix_in_list(const char *option, const char **list) +optionalt +prefix_in_list(const std::string &option, const std::vector &list) { - for(std::size_t i = 0; list[i] != nullptr; i++) - { - if(strncmp(option, list[i], strlen(list[i])) == 0) - { - return {list[i]}; - } - } - - return {}; + const auto found = + std::find_if(list.cbegin(), list.cend(), [&](const std::string &argument) { + return has_prefix(argument, option); + }); + if(found == list.cend()) + return {}; + return {*found}; } bool armcc_cmdlinet::parse(int argc, const char **argv) From f7787c80e3457f537f2f42cea45af4d0d1361281 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 10 Aug 2020 12:13:29 +0100 Subject: [PATCH 8/8] Add unit test of `prefix_in_list` To demonstrate that the refactored version works as expected. --- unit/CMakeLists.txt | 1 + unit/Makefile | 3 +++ unit/goto-cc/armcc_cmdline.cpp | 34 ++++++++++++++++++++++++++++ unit/goto-cc/module_dependencies.txt | 3 +++ 4 files changed, 41 insertions(+) create mode 100644 unit/goto-cc/armcc_cmdline.cpp create mode 100644 unit/goto-cc/module_dependencies.txt diff --git a/unit/CMakeLists.txt b/unit/CMakeLists.txt index 6372702081a..bcc7f37fc08 100644 --- a/unit/CMakeLists.txt +++ b/unit/CMakeLists.txt @@ -51,6 +51,7 @@ target_link_libraries( testing-utils ansi-c solvers + goto-cc-lib goto-checker goto-programs goto-instrument-lib diff --git a/unit/Makefile b/unit/Makefile index d3625d05fa8..246c090bca7 100644 --- a/unit/Makefile +++ b/unit/Makefile @@ -19,6 +19,7 @@ SRC += analyses/ai/ai.cpp \ big-int/big-int.cpp \ compound_block_locations.cpp \ get_goto_model_from_c_test.cpp \ + goto-cc/armcc_cmdline.cpp \ goto-checker/report_util/is_property_less_than.cpp \ goto-instrument/cover_instrument.cpp \ goto-instrument/cover/cover_only.cpp \ @@ -162,6 +163,8 @@ testing-utils-clean: BMC_DEPS =../src/cbmc/c_test_input_generator$(OBJEXT) \ ../src/cbmc/cbmc_languages$(OBJEXT) \ ../src/cbmc/cbmc_parse_options$(OBJEXT) \ + ../src/goto-cc/armcc_cmdline$(OBJEXT) \ + ../src/goto-cc/goto_cc_cmdline$(OBJEXT) \ ../src/goto-instrument/source_lines$(OBJEXT) \ ../src/goto-instrument/cover$(OBJEXT) \ ../src/goto-instrument/cover_basic_blocks$(OBJEXT) \ diff --git a/unit/goto-cc/armcc_cmdline.cpp b/unit/goto-cc/armcc_cmdline.cpp new file mode 100644 index 00000000000..e9d9ec4f154 --- /dev/null +++ b/unit/goto-cc/armcc_cmdline.cpp @@ -0,0 +1,34 @@ +/// \file +/// Unit tests of src/goto-cc/armcc_cmdline.cpp +/// \author Diffblue Ltd. + +#include + +#include + +#include +#include + +optionalt +prefix_in_list(const std::string &option, const std::vector &list); + +static const std::vector test_list{"spam", "eggs", "and", "ham"}; + +TEST_CASE("prefix_in_list exact match", "[core][armcc_cmdline][prefix_in_list]") +{ + REQUIRE(*prefix_in_list("spam", test_list) == "spam"); + REQUIRE(*prefix_in_list("ham", test_list) == "ham"); +} + +TEST_CASE( + "prefix_in_list match prefix", + "[core][armcc_cmdline][prefix_in_list]") +{ + REQUIRE(*prefix_in_list("sp", test_list) == "spam"); + REQUIRE(*prefix_in_list("ha", test_list) == "ham"); +} + +TEST_CASE("prefix_in_list unmatched", "[core][armcc_cmdline][prefix_in_list]") +{ + REQUIRE_FALSE(prefix_in_list("foobar", test_list)); +} diff --git a/unit/goto-cc/module_dependencies.txt b/unit/goto-cc/module_dependencies.txt new file mode 100644 index 00000000000..d670dd22445 --- /dev/null +++ b/unit/goto-cc/module_dependencies.txt @@ -0,0 +1,3 @@ +goto-cc +testing-utils +util