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
