Refactor and modernize the Configurator class, add tests. The changes include: - Use of more appropriate data structures (maps vs. vectors). - More robust configuration format parsing (with warnings). - More robust integer/float/bool parsing (supports wider range of syntaxes). - Unit tests to cover read/parse functionality. Bug: b/216019572 Change-Id: I470b29cfa5d4f901ea7d70d55e39baeba029e363 Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/61649 Reviewed-by: Nicolas Capens <nicolascapens@google.com> Kokoro-Result: kokoro <noreply+kokoro@google.com> Tested-by: Daniele Vettorel <dvet@google.com> Commit-Queue: Daniele Vettorel <dvet@google.com>
diff --git a/src/System/Configurator.cpp b/src/System/Configurator.cpp index 79744c4..57214a4 100644 --- a/src/System/Configurator.cpp +++ b/src/System/Configurator.cpp
@@ -13,47 +13,55 @@ // limitations under the License. #include "Configurator.hpp" +#include "Debug.hpp" -#include <ctype.h> -#include <stdarg.h> -#include <stdio.h> +#include <algorithm> #include <fstream> -#include <iostream> +#include <istream> +#include <sstream> -#if defined(__unix__) -# include <unistd.h> -#endif +namespace { +inline std::string trimSpaces(const std::string &str) +{ + std::string trimmed = str; + trimmed.erase(trimmed.begin(), std::find_if(trimmed.begin(), trimmed.end(), [](unsigned char c) { + return !std::isspace(c); + })); + trimmed.erase(std::find_if(trimmed.rbegin(), trimmed.rend(), [](unsigned char c) { + return !std::isspace(c); + }).base(), + trimmed.end()); + return trimmed; +} +} // namespace namespace sw { -Configurator::Configurator(const std::string &iniPath) +Configurator::Configurator(const std::string &filePath) { - path = iniPath; - - readFile(); -} - -Configurator::~Configurator() -{ -} - -bool Configurator::readFile() -{ -#if defined(__unix__) - if(access(path.c_str(), R_OK) != 0) + std::fstream file(filePath, std::ios::in); + if(file.fail()) { - return false; + return; } -#endif + readConfiguration(file); + file.close(); +} - std::fstream file(path.c_str(), std::ios::in); - if(file.fail()) return false; +Configurator::Configurator(std::istream &str) +{ + readConfiguration(str); +} +bool Configurator::readConfiguration(std::istream &str) +{ std::string line; - std::string keyName; + std::string sectionName; - while(getline(file, line)) + int lineNumber = 0; + while(getline(str, line)) { + ++lineNumber; if(line.length()) { if(line[line.length() - 1] == '\r') @@ -63,8 +71,7 @@ if(!isprint(line[0])) { - // printf("Failing on char %d\n", line[0]); - file.close(); + sw::warn("Cannot parse line %d of configuration, skipping line\n", lineNumber); return false; } @@ -80,16 +87,26 @@ if(pRight != std::string::npos && pRight > pLeft) { - keyName = line.substr(pLeft + 1, pRight - pLeft - 1); - addKeyName(keyName); + sectionName = trimSpaces(line.substr(pLeft + 1, pRight - pLeft - 1)); + if(!sectionName.length()) + { + sw::warn("Found empty section name at line %d of configuration\n", lineNumber); + } } } break; case '=': { - std::string valueName = line.substr(0, pLeft); - std::string value = line.substr(pLeft + 1); - addValue(keyName, valueName, value); + std::string key = trimSpaces(line.substr(0, pLeft)); + std::string value = trimSpaces(line.substr(pLeft + 1)); + if(!key.length() || !value.length()) + { + sw::warn("Cannot parse key-value pair at line %d of configuration (key or value is empty), skipping key-value pair\n", lineNumber); + } + else + { + sections[sectionName].keyValuePairs[key] = value; + } } break; case ';': @@ -101,154 +118,117 @@ } } - file.close(); - - if(names.size()) - { - return true; - } - - return false; + return sections.size() > 0; } -void Configurator::writeFile(const std::string &title) +void Configurator::writeFile(const std::string &filePath, const std::string &title) { -#if defined(__unix__) - if(access(path.c_str(), W_OK) != 0) + std::fstream file(filePath, std::ios::out); + if(file.fail()) { return; } -#endif - - std::fstream file(path.c_str(), std::ios::out); - if(file.fail()) return; file << "; " << title << std::endl << std::endl; - for(unsigned int keyID = 0; keyID < sections.size(); keyID++) + for(const auto &[sectionName, section] : sections) { - file << "[" << names[keyID] << "]" << std::endl; - - for(unsigned int valueID = 0; valueID < sections[keyID].names.size(); valueID++) + file << "[" << sectionName << "]" << std::endl; + for(const auto &[key, value] : section.keyValuePairs) { - file << sections[keyID].names[valueID] << "=" << sections[keyID].values[valueID] << std::endl; + file << key << "=" << value << std::endl; } - file << std::endl; } file.close(); } -int Configurator::findKey(const std::string &keyName) const +std::optional<std::string> Configurator::getValueIfExists(const std::string §ionName, const std::string &keyName) const { - for(unsigned int keyID = 0; keyID < names.size(); keyID++) + const auto section = sections.find(sectionName); + if(section == sections.end()) { - if(names[keyID] == keyName) - { - return keyID; - } + return std::nullopt; } - return -1; -} - -int Configurator::findValue(unsigned int keyID, const std::string &valueName) const -{ - if(!sections.size() || keyID >= sections.size()) + const auto keyValue = section->second.keyValuePairs.find(keyName); + if(keyValue == section->second.keyValuePairs.end()) { - return -1; + return std::nullopt; } - for(unsigned int valueID = 0; valueID < sections[keyID].names.size(); ++valueID) + return keyValue->second; +} + +std::string Configurator::getValue(const std::string §ionName, const std::string &keyName, const std::string &defaultValue) const +{ + const auto value = getValueIfExists(sectionName, keyName); + if(value) { - if(sections[keyID].names[valueID] == valueName) - { - return valueID; - } + return *value; + } + return defaultValue; +} + +void Configurator::addValue(const std::string §ionName, const std::string &keyName, const std::string &value) +{ + sections[sectionName].keyValuePairs[keyName] = value; +} + +int Configurator::getInteger(const std::string §ionName, const std::string &keyName, int defaultValue) const +{ + auto strValue = getValueIfExists(sectionName, keyName); + if(!strValue) + { + return defaultValue; } - return -1; -} - -unsigned int Configurator::addKeyName(const std::string &keyName) -{ - names.resize(names.size() + 1, keyName); - sections.resize(sections.size() + 1); - return (unsigned int)names.size() - 1; -} - -void Configurator::addValue(const std::string &keyName, const std::string &valueName, const std::string &value) -{ - int keyID = findKey(keyName); - - if(keyID == -1) + std::stringstream ss{ *strValue }; + if(strValue->find("0x") != std::string::npos) { - keyID = addKeyName(keyName); + ss >> std::hex; } - int valueID = findValue(keyID, valueName); + int val = 0; + ss >> val; + return val; +} - if(valueID == -1) +bool Configurator::getBoolean(const std::string §ionName, const std::string &keyName, bool defaultValue) const +{ + auto strValue = getValueIfExists(sectionName, keyName); + if(!strValue) { - sections[keyID].names.resize(sections[keyID].names.size() + 1, valueName); - sections[keyID].values.resize(sections[keyID].values.size() + 1, value); + return defaultValue; } - else + + std::stringstream ss{ *strValue }; + + bool val; + ss >> val; + if(ss.fail()) { - sections[keyID].values[valueID] = value; + // Accept "true" and "false" as well. + ss.clear(); + ss >> std::boolalpha >> val; } + return val; } -std::string Configurator::getValue(const std::string &keyName, const std::string &valueName, const std::string &defaultValue) const +double Configurator::getFloat(const std::string §ionName, const std::string &keyName, double defaultValue) const { - int keyID = findKey(keyName); - if(keyID == -1) return defaultValue; - int valueID = findValue((unsigned int)keyID, valueName); - if(valueID == -1) return defaultValue; + auto strValue = getValueIfExists(sectionName, keyName); + if(!strValue) + { + return defaultValue; + } - return sections[keyID].values[valueID]; + std::stringstream ss{ *strValue }; + + double val = 0; + ss >> val; + return val; } - -int Configurator::getInteger(const std::string &keyName, const std::string &valueName, int defaultValue) const -{ - char svalue[256]; - - sprintf(svalue, "%d", defaultValue); - - return atoi(getValue(keyName, valueName, svalue).c_str()); -} - -bool Configurator::getBoolean(const std::string &keyName, const std::string &valueName, bool defaultValue) const -{ - return getInteger(keyName, valueName, (int)defaultValue) != 0; -} - -double Configurator::getFloat(const std::string &keyName, const std::string &valueName, double defaultValue) const -{ - char svalue[256]; - - sprintf(svalue, "%f", defaultValue); - - return atof(getValue(keyName, valueName, svalue).c_str()); -} - -unsigned int Configurator::getFormatted(const std::string &keyName, const std::string &valueName, char *format, - void *v1, void *v2, void *v3, void *v4, - void *v5, void *v6, void *v7, void *v8, - void *v9, void *v10, void *v11, void *v12, - void *v13, void *v14, void *v15, void *v16) -{ - std::string value = getValue(keyName, valueName); - - if(!value.length()) return false; - - unsigned int nVals = sscanf(value.c_str(), format, - v1, v2, v3, v4, v5, v6, v7, v8, - v9, v10, v11, v12, v13, v14, v15, v16); - - return nVals; -} - } // namespace sw
diff --git a/src/System/Configurator.hpp b/src/System/Configurator.hpp index 2d8a61a..6c1aa77 100644 --- a/src/System/Configurator.hpp +++ b/src/System/Configurator.hpp
@@ -15,51 +15,40 @@ #ifndef sw_Configurator_hpp #define sw_Configurator_hpp +#include <optional> #include <string> -#include <vector> - -#include <stdlib.h> +#include <unordered_map> namespace sw { class Configurator { public: - Configurator(const std::string &iniPath = ""); + // Construct a Configurator given a configuration file. + explicit Configurator(const std::string &filePath); - ~Configurator(); + // Construct a Configurator given an in-memory stream. + explicit Configurator(std::istream &str); - std::string getValue(const std::string §ionName, const std::string &valueName, const std::string &defaultValue = "") const; - int getInteger(const std::string §ionName, const std::string &valueName, int defaultValue = 0) const; - bool getBoolean(const std::string §ionName, const std::string &valueName, bool defaultValue = false) const; - double getFloat(const std::string §ionName, const std::string &valueName, double defaultValue = 0.0) const; - unsigned int getFormatted(const std::string §ionName, const std::string &valueName, char *format, - void *v1 = 0, void *v2 = 0, void *v3 = 0, void *v4 = 0, - void *v5 = 0, void *v6 = 0, void *v7 = 0, void *v8 = 0, - void *v9 = 0, void *v10 = 0, void *v11 = 0, void *v12 = 0, - void *v13 = 0, void *v14 = 0, void *v15 = 0, void *v16 = 0); + void writeFile(const std::string &filePath, const std::string &title = "Configuration File"); - void addValue(const std::string §ionName, const std::string &valueName, const std::string &value); + int getInteger(const std::string §ionName, const std::string &keyName, int defaultValue = 0) const; + bool getBoolean(const std::string §ionName, const std::string &keyName, bool defaultValue = false) const; + double getFloat(const std::string §ionName, const std::string &keyName, double defaultValue = 0.0) const; - void writeFile(const std::string &title = "Configuration File"); + std::string getValue(const std::string §ionName, const std::string &keyName, const std::string &defaultValue = "") const; + void addValue(const std::string §ionName, const std::string &keyName, const std::string &value); private: - bool readFile(); + bool readConfiguration(std::istream &str); - unsigned int addKeyName(const std::string §ionName); - int findKey(const std::string §ionName) const; - int findValue(unsigned int sectionID, const std::string &valueName) const; - - std::string path; + std::optional<std::string> getValueIfExists(const std::string §ionName, const std::string &keyName) const; struct Section { - std::vector<std::string> names; - std::vector<std::string> values; + std::unordered_map<std::string, std::string> keyValuePairs; }; - - std::vector<Section> sections; - std::vector<std::string> names; + std::unordered_map<std::string, Section> sections; }; } // namespace sw
diff --git a/tests/SystemUnitTests/BUILD.gn b/tests/SystemUnitTests/BUILD.gn index b3256e0..d917eda 100644 --- a/tests/SystemUnitTests/BUILD.gn +++ b/tests/SystemUnitTests/BUILD.gn
@@ -26,6 +26,7 @@ sources = [ "//gpu/swiftshader_tests_main.cc", + "ConfiguratorTests.cpp", "LRUCacheTests.cpp", "unittests.cpp", "SynchronizationTests.cpp",
diff --git a/tests/SystemUnitTests/CMakeLists.txt b/tests/SystemUnitTests/CMakeLists.txt index 6871d53..dfb7b13 100644 --- a/tests/SystemUnitTests/CMakeLists.txt +++ b/tests/SystemUnitTests/CMakeLists.txt
@@ -23,6 +23,7 @@ ) set(SYSTEM_UNIT_TESTS_SRC_FILES + ConfiguratorTests.cpp LRUCacheTests.cpp main.cpp unittests.cpp
diff --git a/tests/SystemUnitTests/ConfiguratorTests.cpp b/tests/SystemUnitTests/ConfiguratorTests.cpp new file mode 100644 index 0000000..c019152 --- /dev/null +++ b/tests/SystemUnitTests/ConfiguratorTests.cpp
@@ -0,0 +1,124 @@ +// Copyright 2022 The SwiftShader Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "System/Configurator.hpp" + +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include <cstdlib> +#include <sstream> + +using namespace sw; + +TEST(Configurator, IntegerOptionsAreParsedCorrectly) +{ + std::istringstream config{ R"( + [SectionA] + OptionA = 8 + OptionB = 0xff + OptionC = -10 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getInteger("SectionA", "OptionA", 0), 8); + EXPECT_EQ(configurator.getInteger("SectionA", "OptionB", 0), 255); + EXPECT_EQ(configurator.getInteger("SectionA", "OptionC", 0), -10); +} + +TEST(Configurator, FloatOptionsAreParsedCorrectly) +{ + std::istringstream config{ R"( + [SectionA] + OptionA = 1.25 + OptionB = 3 + OptionC = 1e2 + OptionD = -1.5 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getFloat("SectionA", "OptionA", 0.0f), 1.25f); + EXPECT_EQ(configurator.getFloat("SectionA", "OptionB", 0.0f), 3.0f); + EXPECT_EQ(configurator.getFloat("SectionA", "OptionC", 0.0f), 100.0f); + EXPECT_EQ(configurator.getFloat("SectionA", "OptionD", 0.0f), -1.5f); +} + +TEST(Configurator, BooleanOptionsAreParsedCorrectly) +{ + std::istringstream config{ R"( + [SectionA] + OptionA = true + OptionB = false + OptionC = 1 + OptionD = 0 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getBoolean("SectionA", "OptionA", false), true); + EXPECT_EQ(configurator.getBoolean("SectionA", "OptionB", true), false); + EXPECT_EQ(configurator.getBoolean("SectionA", "OptionC", false), true); + EXPECT_EQ(configurator.getBoolean("SectionA", "OptionD", true), false); +} + +TEST(Configurator, MultipleSectionsSameKeyAreDistinguished) +{ + std::istringstream config{ R"( + [SectionA] + OptionA = 1 + + [SectionB] + OptionA = 2 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getInteger("SectionA", "OptionA", 0), 1); + EXPECT_EQ(configurator.getInteger("SectionB", "OptionA", 0), 2); +} + +TEST(Configurator, SameKeyRepeatedHasLastValue) +{ + std::istringstream config{ R"( + [SectionA] + OptionA = 1 + OptionA = 2 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getInteger("SectionA", "OptionA", 0), 2); +} + +TEST(Configurator, NonExistentKeyReturnsDefault) +{ + std::istringstream config{ R"( + [SectionA] + OptionA = 1 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getInteger("SectionA", "NonExistentOption", 123), 123); + EXPECT_EQ(configurator.getFloat("SectionA", "NonExistentOption", 1.5f), 1.5f); + EXPECT_EQ(configurator.getBoolean("SectionA", "NonExistentOption", true), true); +} + +TEST(Configurator, SectionlessOptions) +{ + std::istringstream config{ R"( + OptionA = 8 + OptionB = 1.5 + )" }; + Configurator configurator{ config }; + + EXPECT_EQ(configurator.getInteger("", "OptionA", 0), 8); + EXPECT_EQ(configurator.getFloat("", "OptionB", 0), 1.5f); +} \ No newline at end of file