From ea756ef646ea0f99c5a8931a04f44d3b23a4eadb Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Sun, 30 Aug 2015 23:28:40 +0200 Subject: [PATCH 1/4] Utility: enhance asString usage asString was not returning it's result, leading to more complicated usage. Return the formated string instead of using an output parameter. Signed-off-by: Kevin Rocard --- parameter/MappingData.cpp | 6 +---- parameter/SystemClass.cpp | 6 ++--- parameter/log/include/log/LogWrapper.h | 3 +-- test/test-platform/main.cpp | 4 +-- utility/Utility.cpp | 37 +++++++++----------------- utility/Utility.h | 19 +++++++++---- 6 files changed, 31 insertions(+), 44 deletions(-) diff --git a/parameter/MappingData.cpp b/parameter/MappingData.cpp index dbf7a12be..0d86c28c4 100644 --- a/parameter/MappingData.cpp +++ b/parameter/MappingData.cpp @@ -91,11 +91,7 @@ bool CMappingData::getValue(const std::string& strkey, const std::string*& pStrV std::string CMappingData::asString() const { - std::string strValue; - - CUtility::asString(_keyToValueMap, strValue, ", ", ":"); - - return strValue; + return CUtility::asString(_keyToValueMap, ", ", ":"); } bool CMappingData::addValue(const std::string& strkey, const std::string& strValue) diff --git a/parameter/SystemClass.cpp b/parameter/SystemClass.cpp index a30286cc2..d56abea4b 100644 --- a/parameter/SystemClass.cpp +++ b/parameter/SystemClass.cpp @@ -151,10 +151,8 @@ bool CSystemClass::loadSubsystemsFromSharedLibraries(core::Results& errors, if (!lstrPluginFiles.empty()) { // Unable to load at least one plugin - string strPluginUnloaded; - CUtility::asString(lstrPluginFiles, strPluginUnloaded, ", "); - - errors.push_back("Unable to load the following plugins: " + strPluginUnloaded + "."); + errors.push_back("Unable to load the following plugins: " + + CUtility::asString(lstrPluginFiles, ", ") + "."); return false; } diff --git a/parameter/log/include/log/LogWrapper.h b/parameter/log/include/log/LogWrapper.h index 79980c7ab..76c96801b 100644 --- a/parameter/log/include/log/LogWrapper.h +++ b/parameter/log/include/log/LogWrapper.h @@ -100,9 +100,8 @@ class LogWrapper */ LogWrapper& operator<<(const std::list& logs) { - std::string formatedLogs; std::string separator = "\n" + mProlog; - CUtility::asString(logs, formatedLogs, separator); + std::string formatedLogs = CUtility::asString(logs, separator); // Check if there is something in the log to know if we have to add a prefix if (!mLog.str().empty() && mLog.str()[mLog.str().length() - 1] == separator[0]) { diff --git a/test/test-platform/main.cpp b/test/test-platform/main.cpp index 66746de18..74d9b93c5 100644 --- a/test/test-platform/main.cpp +++ b/test/test-platform/main.cpp @@ -102,9 +102,7 @@ int main(int argc, char *argv[]) // All arguments should have been consumed if (not options.empty()) { - std::string extraArgs; - CUtility::asString(options, extraArgs); - showInvalidUsage("Unexpected extra arguments: " + extraArgs); + showInvalidUsage("Unexpected extra arguments: " + CUtility::asString(options)); return 3; } diff --git a/utility/Utility.cpp b/utility/Utility.cpp index 3ce6318aa..56e02c0c5 100644 --- a/utility/Utility.cpp +++ b/utility/Utility.cpp @@ -32,45 +32,32 @@ #include #include -#include +#include using std::string; // Format string list -void CUtility::asString(const std::list& lstr, - std::string& strOutput, +std::string CUtility::asString(const std::list& lstr, const std::string& strSeparator) { - std::ostringstream ostrFormatedList; - - std::copy(lstr.begin(), lstr.end(), - std::ostream_iterator(ostrFormatedList, strSeparator.c_str())); - - strOutput = ostrFormatedList.str(); - - // Remove last separator - if (strOutput.size() > strSeparator.size()) { - - strOutput.erase(strOutput.size() - strSeparator.size()); - } + return accumulate1(begin(lstr), end(lstr), + [strSeparator](string acc, string right) { + return acc + strSeparator + right; + }); } // Format string map -void CUtility::asString(const std::map& mapStr, - std::string& strOutput, - const std::string& strItemSeparator, - const std::string& strKeyValueSeparator) +std::string CUtility::asString(const std::map& mapStr, + const std::string& strItemSeparator, + const std::string& strKeyValueSeparator) { std::list listKeysValues; - std::map::const_iterator iter; - - for(iter = mapStr.begin(); iter != mapStr.end(); ++iter) { - - listKeysValues.push_back(iter->first + strKeyValueSeparator + iter->second); + for (const auto & item: mapStr) { + listKeysValues.emplace_back(item.first + strKeyValueSeparator + item.second); } - CUtility::asString(listKeysValues, strOutput, strItemSeparator); + return asString(listKeysValues, strItemSeparator); } void CUtility::appendTitle(string& strTo, const string& strTitle) diff --git a/utility/Utility.h b/utility/Utility.h index 38afc2937..dcc5b96f6 100644 --- a/utility/Utility.h +++ b/utility/Utility.h @@ -34,20 +34,30 @@ #include #include #include +#include class CUtility { public: + template + static T accumulate1(InputIt first, InputIt last, BinaryOperation op, T empty = T{}) + { + if (first == last) { return empty; } + auto init = *first++; + + return std::accumulate(first, last, init, op); + } + /** * Format the items of a map into a string as a list of key-value pairs. The map must be * composed of pairs of strings. * * @param[in] mapStr A map of strings - * @param[out] strOutput The output string * @param[in] separator The separator to use between each item + * + * @return the concatenated elements. */ - static void asString(const std::list& lstr, - std::string& strOutput, + static std::string asString(const std::list& lstr, const std::string& separator = "\n"); /** @@ -59,8 +69,7 @@ class CUtility * @param[in] strItemSeparator The separator to use between each item (key-value pair) * @param[in] strKeyValueSeparator The separator to use between key and value */ - static void asString(const std::map& mapStr, - std::string& strOutput, + static std::string asString(const std::map& mapStr, const std::string& strItemSeparator = ", ", const std::string& strKeyValueSeparator = ":"); From 09c87da834dbdb069b537865c960d3e826516091 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Thu, 10 Sep 2015 14:41:41 +0200 Subject: [PATCH 2/4] Utility: Introduce unit test for Utility.h Utility functions were not tested, introduce unit tests for all functions in Utility.h. Signed-off-by: Kevin Rocard --- utility/CMakeLists.txt | 12 +++ utility/test/utility.cpp | 172 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 utility/test/utility.cpp diff --git a/utility/CMakeLists.txt b/utility/CMakeLists.txt index e87148277..7ca98d62e 100644 --- a/utility/CMakeLists.txt +++ b/utility/CMakeLists.txt @@ -52,3 +52,15 @@ install(FILES Utility.h convert.hpp DESTINATION "include/utility") + +if(BUILD_TESTING) + # Add unit test + add_executable(utilityUnitTest test/utility.cpp) + + target_link_libraries(utilityUnitTest pfw_utility catch) + add_test(NAME utilityUnitTest + COMMAND utilityUnitTest) + + # Custom function defined in the top-level CMakeLists + set_test_env(utilityUnitTest) +endif() diff --git a/utility/test/utility.cpp b/utility/test/utility.cpp new file mode 100644 index 000000000..b9d22b602 --- /dev/null +++ b/utility/test/utility.cpp @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2011-2014, Intel Corporation + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without modification, + * are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation and/or + * other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include "Utility.h" +#include +#include + +using std::list; +using std::string; + +SCENARIO("accumulate1") { + struct Test + { + list input; + std::function binaryOpt; + int empty; + int result; + int resultNoEmpty; + }; + const list tests = + { + {{}, nullptr, 21, 21, 0}, + {{5}, nullptr, -1, 5, 5}, + {{5, 2}, [](int, int){return 73;}, -1, 73, 73}, + {{2, 3, 7}, [](int l, int r){return l * r ;}, -1, 42, 42}, + {{1, 10, 100}, [](int l, int r){return l + r ;}, -1, 111, 111} + }; + for (auto &test : tests) { + CAPTURE(Catch::toString(test.input)); + const auto &first = begin(test.input); + const auto &last = end(test.input); + REQUIRE(CUtility::accumulate1(first, last, test.binaryOpt, test.empty) == test.result); + REQUIRE(CUtility::accumulate1(first, last, test.binaryOpt) == test.resultNoEmpty); + } +} + + +SCENARIO("asString(list)") { + struct Test + { + string title; + list input; + string separator; + string result; + string resultNoSep; + }; + const list tests = + { + {"Empty list", {}, "aa", "", ""}, + {"One element", {"a"}, "<>", "a", "a"}, + {"Three elem list", {"1", "2", "3"}, "**", "1**2**3", "1\n2\n3"}, + {"No separator", {"12", "ab", "+-"}, "", "12ab+-", "12\nab\n+-"}, + {"empty elem list", {"a", "b", "", "d"}, "|", "a|b||d", "a\nb\n\nd"}, + }; + for (auto &test : tests) { + CAPTURE(Catch::toString(test.input)); + WHEN("Separator, " + test.title) { + CAPTURE(test.separator); + REQUIRE(CUtility::asString(test.input, test.separator) == test.result); + } + THEN("No separator, " + test.title) { + REQUIRE(CUtility::asString(test.input) == test.resultNoSep); + } + } +} + +SCENARIO("asString(map)") { + using std::map; + + using Map = map; + struct Test + { + Map input; + string itemSep; + string keyValueSep; + string result; + string resultNoKeyValueSep; + string resultNoSep; + }; + const list tests = + { + {{}, "itemSep", "keyValueSep", "", "", ""}, + { Map{{"a", "b"}, + {"c", "d"}, + {"e", "f"}}, // input + " - ", "\n", // item & keyValue sep + "a - b\nc - d\ne - f", //result + "a:b\nc:d\ne:f", //resultNoKeyValueSep + "a:b, c:d, e:f" //resultNoSep + } + }; + for (const auto &test : tests) { + CAPTURE(Catch::toString(test.input)); + CAPTURE(test.keyValueSep); + CAPTURE(test.itemSep); + REQUIRE(CUtility::asString(test.input, test.keyValueSep, test.itemSep) == test.result); + REQUIRE(CUtility::asString(test.input, test.keyValueSep) == test.resultNoKeyValueSep); + REQUIRE(CUtility::asString(test.input) == test.resultNoSep); + } + +} + +std::string quote(std::string toQuote) { return '"' + toQuote + '"'; } + +SCENARIO("appendTitle") { + struct Test + { + string initial; + string title; + string result; + }; + const list tests = + { + {"", "abc", "\nabc\n===\n"}, + {"start", "title", "start\ntitle\n=====\n"} + }; + for (auto &test : tests) { + GIVEN("A title: " + quote(test.title)) { + CAPTURE(test.initial); + CAPTURE(test.title); + + WHEN("Appending to: " + quote(test.initial)) { + string output = test.initial; + THEN("Result should be:\n" + quote(test.result)) { + CUtility::appendTitle(output, test.title); + CHECK(output == test.result); + } + } + } + } +} + +SCENARIO("isNotHexadecimal") { + for (auto &str : {"a", "0", "012", "13", "ABC", "Oxa"}) { + CAPTURE(str); + CHECK(not CUtility::isHexadecimal(str)); + } +} + +SCENARIO("isHexadecimal") { + for (auto str : {"0xa", "0X0", "0x012", "0x13", "0xConsider as hexa as stariting with 0x"}) { + CAPTURE(str); + CHECK(CUtility::isHexadecimal(str)); + } +} From 63d41e6a69a3738fae5443f120330533b77fac61 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Thu, 8 Oct 2015 18:46:36 +0200 Subject: [PATCH 3/4] Utility: Simplify appendTitle implementation Signed-off-by: Kevin Rocard --- utility/Utility.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/utility/Utility.cpp b/utility/Utility.cpp index 56e02c0c5..dbe5a79ea 100644 --- a/utility/Utility.cpp +++ b/utility/Utility.cpp @@ -62,15 +62,8 @@ std::string CUtility::asString(const std::map& mapStr, void CUtility::appendTitle(string& strTo, const string& strTitle) { - strTo += "\n" + strTitle + "\n"; - - size_t uiLength = strTitle.size(); - - while (uiLength--) { - - strTo += "="; - } - strTo += "\n"; + strTo += "\n" + strTitle + "\n" + + string(strTitle.size(), '=') + "\n"; } bool CUtility::isHexadecimal(const string& strValue) From ec04aeb33cbe223e421362c0ad20490392a26cb6 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Fri, 11 Sep 2015 15:37:36 +0200 Subject: [PATCH 4/4] CommandHandle: Simplify help message implementation Use `std::string::string(size_t, char)` to simplify the construction of the help message. Signed-off-by: Kevin Rocard --- remote-processor/RemoteCommandHandlerTemplate.h | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/remote-processor/RemoteCommandHandlerTemplate.h b/remote-processor/RemoteCommandHandlerTemplate.h index 16b6a72c3..b17f34331 100644 --- a/remote-processor/RemoteCommandHandlerTemplate.h +++ b/remote-processor/RemoteCommandHandlerTemplate.h @@ -208,17 +208,11 @@ class TRemoteCommandHandlerTemplate : public IRemoteCommandHandler std::string strUsage = pRemoteCommandParserItem->usage(); - strResult += strUsage; - // Align size_t spacesToAdd = _maxCommandUsageLength + 5 - strUsage.length(); - while (spacesToAdd--) { - - strResult += " "; - } - - strResult += std::string("=> ") + std::string(pRemoteCommandParserItem->getDescription()) + "\n"; + strResult += strUsage + std::string(spacesToAdd, ' ') + "=> " + + pRemoteCommandParserItem->getDescription() + '\n'; } }