From e87edbf6fb1cc80ff01c729a971e56ced5b53943 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 12 Apr 2018 17:41:04 +0100 Subject: [PATCH 1/6] Removing wrong elements from the make file --- unit/Makefile | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/unit/Makefile b/unit/Makefile index 11468e5cf2f..922dd7cea3d 100644 --- a/unit/Makefile +++ b/unit/Makefile @@ -3,10 +3,6 @@ # Source files for test utilities SRC = unit_tests.cpp \ catch_example.cpp \ - util/expr_iterator.cpp \ - util/optional.cpp \ - analyses/call_graph.cpp \ - java_bytecode/java_bytecode_convert_class/convert_abstract_class.cpp \ # Empty last line # Test source files @@ -48,6 +44,7 @@ SRC += unit_tests.cpp \ util/irep.cpp \ util/irep_sharing.cpp \ util/message.cpp \ + util/optional.cpp \ util/parameter_indices.cpp \ util/simplify_expr.cpp \ util/symbol_table.cpp \ From 252c24c8938294e201463d02eb32b7f206d72c91 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 12 Apr 2018 17:42:52 +0100 Subject: [PATCH 2/6] Migrate old string utils unit tests --- CMakeLists.txt | 1 - unit/.gitignore | 1 - unit/CMakeLists.txt | 12 ---- unit/Makefile | 6 +- unit/string_utils.cpp | 50 -------------- unit/util/string_utils/split_string.cpp | 92 +++++++++++++++++++++++++ unit/util/string_utils/strip_string.cpp | 27 ++++++++ 7 files changed, 121 insertions(+), 68 deletions(-) delete mode 100644 unit/string_utils.cpp create mode 100644 unit/util/string_utils/split_string.cpp create mode 100644 unit/util/string_utils/strip_string.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 030fd699e45..6ffa74b22cd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,7 +75,6 @@ set_target_properties( mmcc pointer-analysis solvers - string_utils test-bigint testing-utils unit diff --git a/unit/.gitignore b/unit/.gitignore index aecf381fc5c..d69e63165b4 100644 --- a/unit/.gitignore +++ b/unit/.gitignore @@ -1,5 +1,4 @@ # Unit test binaries miniBDD sharing_node -string_utils unit_tests diff --git a/unit/CMakeLists.txt b/unit/CMakeLists.txt index 21316d73a38..5b4cdf42335 100644 --- a/unit/CMakeLists.txt +++ b/unit/CMakeLists.txt @@ -5,7 +5,6 @@ file(GLOB_RECURSE testing_utils "testing-utils/*.cpp" "testing-utils/*.h") list(REMOVE_ITEM sources # Used in executables ${CMAKE_CURRENT_SOURCE_DIR}/miniBDD.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/string_utils.cpp # Don't build ${CMAKE_CURRENT_SOURCE_DIR}/sharing_map.cpp @@ -63,14 +62,3 @@ target_include_directories(miniBDD target_link_libraries(miniBDD solvers ansi-c) add_test(NAME miniBDD COMMAND $) set_tests_properties(miniBDD PROPERTIES LABELS "CORE;CBMC") - -add_executable(string_utils string_utils.cpp) -target_include_directories(string_utils - PUBLIC - ${CBMC_BINARY_DIR} - ${CBMC_SOURCE_DIR} - ${CMAKE_CURRENT_SOURCE_DIR} -) -target_link_libraries(string_utils solvers ansi-c) -add_test(NAME string_utils COMMAND $) -set_tests_properties(string_utils PROPERTIES LABELS "CORE;CBMC") diff --git a/unit/Makefile b/unit/Makefile index 922dd7cea3d..89b77a5d16f 100644 --- a/unit/Makefile +++ b/unit/Makefile @@ -47,6 +47,8 @@ SRC += unit_tests.cpp \ util/optional.cpp \ util/parameter_indices.cpp \ util/simplify_expr.cpp \ + util/string_utils/split_string.cpp \ + util/string_utils/strip_string.cpp \ util/symbol_table.cpp \ catch_example.cpp \ java_bytecode/java_virtual_functions/virtual_functions.cpp \ @@ -85,7 +87,6 @@ OBJ += $(CPROVER_LIBS) testing-utils/testing-utils$(LIBEXT) TESTS = unit_tests$(EXEEXT) \ miniBDD$(EXEEXT) \ - string_utils$(EXEEXT) \ # Empty last line CLEANFILES = $(TESTS) @@ -104,6 +105,3 @@ unit_tests$(EXEEXT): $(OBJ) miniBDD$(EXEEXT): miniBDD$(OBJEXT) $(CPROVER_LIBS) $(LINKBIN) - -string_utils$(EXEEXT): string_utils$(OBJEXT) $(CPROVER_LIBS) - $(LINKBIN) diff --git a/unit/string_utils.cpp b/unit/string_utils.cpp deleted file mode 100644 index b02b1d24674..00000000000 --- a/unit/string_utils.cpp +++ /dev/null @@ -1,50 +0,0 @@ -#include -#include -#include - -#include - -int main() -{ - assert(strip_string(" x y ")=="x y"); - - const std::string test=" a,, x , ,"; - - std::vector result; - split_string(test, ',', result, false, false); - assert(result.size()==5); - assert(result[0]==" a"); - assert(result[1]==""); - assert(result[2]==" x "); - assert(result[3]==" "); - assert(result[4]==""); - - result.clear(); - split_string(test, ',', result, true, false); - assert(result.size()==5); - assert(result[0]=="a"); - assert(result[1]==""); - assert(result[2]=="x"); - assert(result[3]==""); - assert(result[4]==""); - - result.clear(); - split_string(test, ',', result, false, true); - assert(result.size()==3); - assert(result[0]==" a"); - assert(result[1]==" x "); - assert(result[2]==" "); - - result.clear(); - split_string(test, ',', result, true, true); - assert(result.size()==2); - assert(result[0]=="a"); - assert(result[1]=="x"); - - std::string s1; - std::string s2; - split_string("a:b", ':', s1, s2, false); - assert(s1=="a"); - assert(s2=="b"); -} - diff --git a/unit/util/string_utils/split_string.cpp b/unit/util/string_utils/split_string.cpp new file mode 100644 index 00000000000..d41d4bbd310 --- /dev/null +++ b/unit/util/string_utils/split_string.cpp @@ -0,0 +1,92 @@ +/* + Author: Diffblue Ltd. +*/ + +/// \file +/// split_string Unit Tests + +#include +#include + +SCENARIO("split_string", "[core][utils][string_utils][split_string]") +{ + GIVEN("A non whitespace delimited string with two elements") + { + const char delimiter = ','; + const std::string string = " a,, x , ,"; + + WHEN("Not stripping, not removing empty") + { + std::vector result; + split_string(string, delimiter, result, false, false); + + THEN("All the elements should remain") + { + std::vector expected_result = {" a", "", " x ", " ", ""}; + REQUIRE_THAT( + result, + Catch::Matchers::Vector::EqualsMatcher{expected_result}); + } + } + WHEN("Stripping, not removing empty") + { + std::vector result; + split_string(string, delimiter, result, true, false); + + THEN("All whitespace borders should be removed but all elements remain") + { + std::vector expected_result = {"a", "", "x", "", ""}; + REQUIRE_THAT( + result, + Catch::Matchers::Vector::EqualsMatcher{expected_result}); + } + } + WHEN("Not stripping, removing empty") + { + std::vector result; + split_string(string, delimiter, result, false, true); + + THEN("All empty elements should be removed") + { + std::vector expected_result = {" a", " x ", " "}; + REQUIRE_THAT( + result, + Catch::Matchers::Vector::EqualsMatcher{expected_result}); + } + } + WHEN("Stripping and removing empty") + { + std::vector result; + split_string(string, delimiter, result, true, true); + + THEN("Should get the two parts in the vector") + { + std::vector expected_result = {"a", "x"}; + REQUIRE_THAT( + result, + Catch::Matchers::Vector::EqualsMatcher{expected_result}); + } + } + } +} + +SCENARIO("split_string into two", "[core][utils][string_utils][split_string]") +{ + GIVEN("A string with one separator") + { + const char separator = ':'; + std::string string = "a:b"; + + WHEN("Splitting into two strings") + { + std::string s1; + std::string s2; + split_string(string, separator, s1, s2, false); + THEN("The string should be split") + { + REQUIRE(s1 == "a"); + REQUIRE(s2 == "b"); + } + } + } +} diff --git a/unit/util/string_utils/strip_string.cpp b/unit/util/string_utils/strip_string.cpp new file mode 100644 index 00000000000..61dbbffc918 --- /dev/null +++ b/unit/util/string_utils/strip_string.cpp @@ -0,0 +1,27 @@ +/* + Author: Diffblue Ltd. +*/ + +/// \file +/// strip_string Unit Tests + +#include +#include + +SCENARIO("strip_string", "[core][utils][string_utils][strip_string]") +{ + GIVEN("A string with some whitespace in it") + { + std::string string = " x y "; + WHEN("Using strip_string") + { + std::string result = strip_string(string); + THEN( + "Whitespace at either end should be removed, but not internal " + "whitespace") + { + REQUIRE(result == "x y"); + } + } + } +} From 07009d450482c3183ed37afd03d586904b504c99 Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 16 Apr 2018 11:17:58 +0100 Subject: [PATCH 3/6] Refactored test to run all combinations --- unit/util/string_utils/split_string.cpp | 129 +++++++++++++++--------- 1 file changed, 81 insertions(+), 48 deletions(-) diff --git a/unit/util/string_utils/split_string.cpp b/unit/util/string_utils/split_string.cpp index d41d4bbd310..299b5577d54 100644 --- a/unit/util/string_utils/split_string.cpp +++ b/unit/util/string_utils/split_string.cpp @@ -8,68 +8,101 @@ #include #include -SCENARIO("split_string", "[core][utils][string_utils][split_string]") +struct expected_resultst { - GIVEN("A non whitespace delimited string with two elements") + std::vector no_strip_no_remove; + std::vector strip_no_remove; + std::vector no_strip_remove_empty; + std::vector strip_remove_empty; +}; + +/// For a given string and delimiter, use the split_string function with all +/// the possible combinations of stripping whitespace and removing empty +/// elements. +/// \param string: The string to split +/// \param delimiter: The delimter to split on +/// \param expected_results: The results expected for each of the versions of +/// the method +void run_on_all_variants( + std::string string, + char delimiter, + const expected_resultst &expected_results) +{ + WHEN("Not stripping, not removing empty") { - const char delimiter = ','; - const std::string string = " a,, x , ,"; + std::vector result; + split_string(string, delimiter, result, false, false); - WHEN("Not stripping, not removing empty") + THEN("Should get expected vector") { - std::vector result; - split_string(string, delimiter, result, false, false); - - THEN("All the elements should remain") - { - std::vector expected_result = {" a", "", " x ", " ", ""}; - REQUIRE_THAT( - result, - Catch::Matchers::Vector::EqualsMatcher{expected_result}); - } + REQUIRE_THAT( + result, + // NOLINTNEXTLINE(whitespace/braces) + Catch::Matchers::Vector::EqualsMatcher{ + expected_results.no_strip_no_remove}); } - WHEN("Stripping, not removing empty") - { - std::vector result; - split_string(string, delimiter, result, true, false); + } + WHEN("Not stripping, removing empty") + { + std::vector result; + split_string(string, delimiter, result, false, true); - THEN("All whitespace borders should be removed but all elements remain") - { - std::vector expected_result = {"a", "", "x", "", ""}; - REQUIRE_THAT( - result, - Catch::Matchers::Vector::EqualsMatcher{expected_result}); - } - } - WHEN("Not stripping, removing empty") + THEN("Should get expected vector") { - std::vector result; - split_string(string, delimiter, result, false, true); - - THEN("All empty elements should be removed") - { - std::vector expected_result = {" a", " x ", " "}; - REQUIRE_THAT( - result, - Catch::Matchers::Vector::EqualsMatcher{expected_result}); - } + REQUIRE_THAT( + result, + // NOLINTNEXTLINE(whitespace/braces) + Catch::Matchers::Vector::EqualsMatcher{ + expected_results.no_strip_remove_empty}); } - WHEN("Stripping and removing empty") + } + WHEN("Stripping, not removing empty") + { + std::vector result; + split_string(string, delimiter, result, true, false); + + THEN("Should get expected vector") { - std::vector result; - split_string(string, delimiter, result, true, true); + REQUIRE_THAT( + result, + // NOLINTNEXTLINE(whitespace/braces) + Catch::Matchers::Vector::EqualsMatcher{ + expected_results.strip_no_remove}); + } + } + WHEN("Stripping and removing empty") + { + std::vector result; + split_string(string, delimiter, result, true, true); - THEN("Should get the two parts in the vector") - { - std::vector expected_result = {"a", "x"}; - REQUIRE_THAT( - result, - Catch::Matchers::Vector::EqualsMatcher{expected_result}); - } + THEN("Should get expected vector") + { + REQUIRE_THAT( + result, + // NOLINTNEXTLINE(whitespace/braces) + Catch::Matchers::Vector::EqualsMatcher{ + expected_results.strip_remove_empty}); } } } +SCENARIO("split_string", "[core][utils][string_utils][split_string]") +{ + GIVEN("A non whitespace delimited string with two elements") + { + const char delimiter = ','; + const std::string string = " a,, x , ,"; + + expected_resultst expected_results; + expected_results.no_strip_no_remove = {" a", "", " x ", " ", ""}; + expected_results.strip_no_remove = {"a", "", "x", "", ""}; + expected_results.no_strip_remove_empty = {" a", " x ", " "}; + expected_results.strip_remove_empty = {"a", "x"}; + + run_on_all_variants(string, delimiter, expected_results); + } +} + SCENARIO("split_string into two", "[core][utils][string_utils][split_string]") { GIVEN("A string with one separator") From 145f474c089e1cf82ad261534ad382587ac7da1e Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 16 Apr 2018 11:28:44 +0100 Subject: [PATCH 4/6] Adding tests for empty string edge cases --- unit/util/string_utils/split_string.cpp | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/unit/util/string_utils/split_string.cpp b/unit/util/string_utils/split_string.cpp index 299b5577d54..ce3beb55b48 100644 --- a/unit/util/string_utils/split_string.cpp +++ b/unit/util/string_utils/split_string.cpp @@ -101,6 +101,39 @@ SCENARIO("split_string", "[core][utils][string_utils][split_string]") run_on_all_variants(string, delimiter, expected_results); } + GIVEN("An empty string") + { + std::string string = ""; + WHEN("Splitting it") + { + expected_resultst expected_results; + expected_results.no_strip_no_remove = {""}; + expected_results.strip_no_remove = {""}; + + // TODO(tkiley): This is probably wrong, since I'd expect removing empty + // TODO(tkiley): elements to return an empty vector here. + expected_results.no_strip_remove_empty = {""}; + expected_results.strip_remove_empty = {""}; + + run_on_all_variants(string, ',', expected_results); + } + } + GIVEN("A whitespace only string") + { + std::string string = " "; + WHEN("Splitting it") + { + expected_resultst expected_results; + expected_results.no_strip_no_remove = {" "}; + expected_results.strip_no_remove = {""}; + expected_results.no_strip_remove_empty = {" "}; + // TODO(tkiley): This is probably wrong, since I'd expect removing empty + // TODO(tkiley): elements to return an empty vector here. + expected_results.strip_remove_empty = {""}; + + run_on_all_variants(string, ',', expected_results); + } + } } SCENARIO("split_string into two", "[core][utils][string_utils][split_string]") From a385d9ba9411863abd5c22fef26ecb23bfe6fa45 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 12 Apr 2018 12:03:30 +0100 Subject: [PATCH 5/6] Allowed split_string to be used on whitespace if not also trying to strip Also improved comments (and moved to doxygen) as I assumed strip would remove whitespace from throughout the string but actually only does the final string Replace asserts in string utils Adding tests for behaviour with whitespace delimiter --- src/util/string_utils.cpp | 31 ++++++++-- src/util/string_utils.h | 6 +- unit/util/string_utils/split_string.cpp | 79 +++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/src/util/string_utils.cpp b/src/util/string_utils.cpp index 62ea151e3b6..c4f81fad315 100644 --- a/src/util/string_utils.cpp +++ b/src/util/string_utils.cpp @@ -7,11 +7,16 @@ Author: Daniel Poetzl \*******************************************************************/ #include "string_utils.h" +#include "invariant.h" #include #include #include +/// Remove all whitespace characters from either end of a string. Whitespace +/// in the middle of the string is left unchanged +/// \param s: the string to strip +/// \return The stripped string std::string strip_string(const std::string &s) { auto pred=[](char c){ return std::isspace(c); }; @@ -30,6 +35,16 @@ std::string strip_string(const std::string &s) return s.substr(i, (j-i+1)); } +/// Given a string s, split into a sequence of substrings when separated by +/// specified delimiter. +/// \param s: The string to split up +/// \param delim: The character to use as the delimiter +/// \param [out] result: The sub strings. Must be empty. +/// \param strip: If true, strip_string will be used on each element, removing +/// whitespace from the beginning and end of each element +/// \param remove_empty: If true, all empty-string elements will be removed. +/// This is applied after strip so whitespace only elements will be removed if +/// both are set to true void split_string( const std::string &s, char delim, @@ -37,8 +52,12 @@ void split_string( bool strip, bool remove_empty) { - assert(result.empty()); - assert(!std::isspace(delim)); + PRECONDITION(result.empty()); + if(strip && std::isspace(delim)) + { + throw std::invalid_argument( + "delim can't be a space character if using strip"); + } if(s.empty()) { @@ -47,7 +66,7 @@ void split_string( } std::string::size_type n=s.length(); - assert(n>0); + INVARIANT(n > 0, "Empty string case should already be handled"); std::string::size_type start=0; std::string::size_type i; @@ -87,7 +106,11 @@ void split_string( std::string &right, bool strip) { - assert(!std::isspace(delim)); + if(strip && std::isspace(delim)) + { + throw std::invalid_argument( + "delim can't be a space character if using strip"); + } std::vector result; diff --git a/src/util/string_utils.h b/src/util/string_utils.h index 35db42a778d..87dd742d666 100644 --- a/src/util/string_utils.h +++ b/src/util/string_utils.h @@ -18,10 +18,10 @@ std::string strip_string(const std::string &s); void split_string( const std::string &s, - char delim, // must not be a whitespace character + char delim, std::vector &result, - bool strip=false, // strip whitespace from elements - bool remove_empty=false); // remove empty elements + bool strip = false, + bool remove_empty = false); void split_string( const std::string &s, diff --git a/unit/util/string_utils/split_string.cpp b/unit/util/string_utils/split_string.cpp index ce3beb55b48..d379a484ab6 100644 --- a/unit/util/string_utils/split_string.cpp +++ b/unit/util/string_utils/split_string.cpp @@ -134,6 +134,58 @@ SCENARIO("split_string", "[core][utils][string_utils][split_string]") run_on_all_variants(string, ',', expected_results); } } + GIVEN("A whitespace delimter") + { + std::string string = "a\nb\nc"; + const char delimiter = '\n'; + + WHEN("Not stripping, not removing empty") + { + std::vector result; + split_string(string, delimiter, result, false, false); + + THEN("Should get expected vector") + { + std::vector expected_result = {"a", "b", "c"}; + REQUIRE_THAT( + result, + Catch::Matchers::Vector::EqualsMatcher{expected_result}); + } + } + WHEN("Not stripping, removing empty") + { + std::vector result; + split_string(string, delimiter, result, false, true); + + THEN("Should get expected vector") + { + std::vector expected_result = {"a", "b", "c"}; + REQUIRE_THAT( + result, + Catch::Matchers::Vector::EqualsMatcher{expected_result}); + } + } + WHEN("Stripping, not removing empty") + { + std::vector result; + THEN("Should throw an exception") + { + REQUIRE_THROWS_AS( + split_string(string, delimiter, result, true, false), + std::invalid_argument); + } + } + WHEN("Stripping and removing empty") + { + std::vector result; + THEN("Should throw an exception") + { + REQUIRE_THROWS_AS( + split_string(string, delimiter, result, true, true), + std::invalid_argument); + } + } + } } SCENARIO("split_string into two", "[core][utils][string_utils][split_string]") @@ -155,4 +207,31 @@ SCENARIO("split_string into two", "[core][utils][string_utils][split_string]") } } } + GIVEN("A string and a whitespace delimiter") + { + std::string string = "a\nb"; + const char delimiter = '\n'; + + WHEN("Splitting in two and not stripping") + { + std::string s1; + std::string s2; + split_string(string, delimiter, s1, s2, false); + THEN("The string should be split") + { + REQUIRE(s1 == "a"); + REQUIRE(s2 == "b"); + } + } + WHEN("Splitting in two and stripping") + { + THEN("An invalid argument exception should be raised") + { + std::string s1; + std::string s2; + REQUIRE_THROWS_AS( + split_string(string, delimiter, s1, s2, true), std::invalid_argument); + } + } + } } From fc8ba8897123bd80466c96fdbdf283c936ce5092 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 19 Apr 2018 10:59:50 +0100 Subject: [PATCH 6/6] Revert to aborting precondition for function inputs --- src/util/string_utils.cpp | 14 +++------- unit/util/string_utils/split_string.cpp | 34 +++---------------------- 2 files changed, 8 insertions(+), 40 deletions(-) diff --git a/src/util/string_utils.cpp b/src/util/string_utils.cpp index c4f81fad315..02a71d991b2 100644 --- a/src/util/string_utils.cpp +++ b/src/util/string_utils.cpp @@ -53,11 +53,8 @@ void split_string( bool remove_empty) { PRECONDITION(result.empty()); - if(strip && std::isspace(delim)) - { - throw std::invalid_argument( - "delim can't be a space character if using strip"); - } + // delim can't be a space character if using strip + PRECONDITION(!std::isspace(delim) || !strip); if(s.empty()) { @@ -106,11 +103,8 @@ void split_string( std::string &right, bool strip) { - if(strip && std::isspace(delim)) - { - throw std::invalid_argument( - "delim can't be a space character if using strip"); - } + // delim can't be a space character if using strip + PRECONDITION(!std::isspace(delim) || !strip); std::vector result; diff --git a/unit/util/string_utils/split_string.cpp b/unit/util/string_utils/split_string.cpp index d379a484ab6..d98d3574a6c 100644 --- a/unit/util/string_utils/split_string.cpp +++ b/unit/util/string_utils/split_string.cpp @@ -165,26 +165,8 @@ SCENARIO("split_string", "[core][utils][string_utils][split_string]") Catch::Matchers::Vector::EqualsMatcher{expected_result}); } } - WHEN("Stripping, not removing empty") - { - std::vector result; - THEN("Should throw an exception") - { - REQUIRE_THROWS_AS( - split_string(string, delimiter, result, true, false), - std::invalid_argument); - } - } - WHEN("Stripping and removing empty") - { - std::vector result; - THEN("Should throw an exception") - { - REQUIRE_THROWS_AS( - split_string(string, delimiter, result, true, true), - std::invalid_argument); - } - } + // TODO(tkiley): here we should check what happens when trying to enable + // TODO(tkiley): strip, but currently the behaviour terminates the unit test } } @@ -223,15 +205,7 @@ SCENARIO("split_string into two", "[core][utils][string_utils][split_string]") REQUIRE(s2 == "b"); } } - WHEN("Splitting in two and stripping") - { - THEN("An invalid argument exception should be raised") - { - std::string s1; - std::string s2; - REQUIRE_THROWS_AS( - split_string(string, delimiter, s1, s2, true), std::invalid_argument); - } - } + // TODO(tkiley): here we should check what happens when trying to enable + // TODO(tkiley): strip, but currently the behaviour terminates the unit test } }