diff options
| -rw-r--r-- | src/configfile.cc | 117 | ||||
| -rw-r--r-- | src/configfile.h | 7 |
2 files changed, 61 insertions, 63 deletions
diff --git a/src/configfile.cc b/src/configfile.cc index 6747090..6a5f176 100644 --- a/src/configfile.cc +++ b/src/configfile.cc @@ -40,30 +40,26 @@ #include <hugin.hpp> +namespace +{ + #if DG_PLATFORM == DG_PLATFORM_WINDOWS -// TODO: lowercase name for non-defines, 'sep'? -static const std::string SEP = "\\"; +const std::string sep = "\\"; #else -static const std::string SEP = "/"; +const std::string sep = "/"; #endif #if DG_PLATFORM == DG_PLATFORM_WINDOWS -// TODO: lowercase name for non-defines, 'config_dir_name'? -static const std::string CONFIGDIRNAME = "DrumGizmo"; +const std::string config_dir_name = "DrumGizmo"; #else // TODO: Should we use this on OSX or is there another standard naming? -static const std::string CONFIGDIRNAME = ".drumgizmo"; +// Response: Apparently this is also fine under OSX. There are also other conventions, but +// one of the is dot files. However, feel free to check this, I am not sure about best practices. +const std::string config_dir_name = ".drumgizmo"; #endif -namespace -{ -// TODO: Put sep and configdirname into anonymous namespace as well instead of -// being static? -// TODO: c++ style comments -/** - * Return the path containing the config files. - */ +// Return the path containing the config files. std::string getConfigPath() { // TODO: Move this utility function as a static function into directory.cc/h? @@ -71,27 +67,23 @@ std::string getConfigPath() std::string configpath; #if DG_PLATFORM == DG_PLATFORM_WINDOWS TCHAR szPath[256]; - // TODO: Compare return value to S_OK instead of using SUCCEEDED macro. - if(SUCCEEDED(SHGetFolderPath(NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE, NULL, 0, szPath))) + if(SHGetFolderPath(NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE, NULL, 0, szPath) == S_OK) { configpath = szPath; } #else configpath = getenv("HOME"); #endif - configpath += SEP; - configpath += CONFIGDIRNAME; + configpath += sep; + configpath += config_dir_name; return configpath; } -/** - * Calling this makes sure that the config path exists - */ +// Calling this makes sure that the config path exists bool createConfigPath() { - // TODO: West-const ;) - std::string const configpath = getConfigPath(); + const std::string configpath = getConfigPath(); struct stat st; if(stat(configpath.c_str(), &st) != 0) @@ -115,8 +107,7 @@ bool createConfigPath() } // end anonymous namespace -// TODO: West-const ;) -ConfigFile::ConfigFile(std::string const& filename) +ConfigFile::ConfigFile(const std::string& filename) : filename(filename) { } @@ -147,12 +138,11 @@ bool ConfigFile::load() // TODO: What happens if the parser encounters a windows newline on linux, or // the other way round? // Perhaps a unit-test for this would be in order? - // TODO: What does line != "" do here? If the newline is part of the string, - // I think it is redundant with getline returning false (end-of-file) - // but if the newline character is stripped from the line, then this - // will make the parser stop if an empty line is encountered in the - // config file. - while(std::getline(current_file, line) && line != "") + // Response: Why? Windows will probably not recognize the newline and Linux will + // read the \r into the end of the line (which was the bug we had). What's the + // point of testing undefined behavior? Do you want to catch it somehow and + // warn the user in this case or what's the idea? + while(std::getline(current_file, line)) { if(!parseLine(line)) { @@ -169,17 +159,17 @@ bool ConfigFile::save() { DEBUG(configfile, "Saving configuration...\n"); - // TODO: Check for failure? It wouldn't make sense to write the file if the - // folder couldn't be created. - createConfigPath(); + if(!createConfigPath()) + { + return false; + } if(!open(std::ios_base::out)) { return false; } - // TODO: West-const ;) - for(auto const& value: values) + for(const auto& value: values) { current_file << value.first << ":" << value.second << std::endl; } @@ -212,7 +202,7 @@ bool ConfigFile::open(std::ios_base::openmode mode) } std::string filename = getConfigPath(); - filename += SEP; + filename += sep; filename += filename; DEBUG(configfile, "Opening config file '%s'\n", filename.c_str()); @@ -223,8 +213,7 @@ bool ConfigFile::open(std::ios_base::openmode mode) bool ConfigFile::parseLine(const std::string& line) { - // TODO: enum class? - enum State + enum class State { before_key, in_key, @@ -236,16 +225,22 @@ bool ConfigFile::parseLine(const std::string& line) after_value, }; + // For empty lines, we parse them fine but don't do anything. + if(line == "") + { + return true; + } + std::string key; std::string value; - State state = before_key; + State state = State::before_key; for(std::size_t pos = 0; pos < line.size(); ++pos) { auto c = line[pos]; switch(state) { - case before_key: + case State::before_key: if(c == '#') { // Comment: Ignore line. @@ -257,31 +252,31 @@ bool ConfigFile::parseLine(const std::string& line) continue; } key += c; - state = in_key; + state = State::in_key; break; - case in_key: + case State::in_key: if(std::isspace(c)) { - state = after_key; + state = State::after_key; continue; } if(c == ':' || c == '=') { - state = before_value; + state = State::before_value; continue; } key += c; break; - case after_key: + case State::after_key: if(std::isspace(c)) { continue; } if(c == ':' || c == '=') { - state = before_value; + state = State::before_value; continue; } ERR(configfile, @@ -290,60 +285,60 @@ bool ConfigFile::parseLine(const std::string& line) line.c_str()); return false; - case before_value: + case State::before_value: if(std::isspace(c)) { continue; } if(c == '\'') { - state = in_value_single_quoted; + state = State::in_value_single_quoted; continue; } if(c == '"') { - state = in_value_double_quoted; + state = State::in_value_double_quoted; continue; } value += c; - state = in_value; + state = State::in_value; break; - case in_value: + case State::in_value: if(std::isspace(c)) { - state = after_value; + state = State::after_value; continue; } if(c == '#') { // Comment: Ignore the rest of the line. pos = line.size(); - state = after_value; + state = State::after_value; continue; } value += c; break; - case in_value_single_quoted: + case State::in_value_single_quoted: if(c == '\'') { - state = after_value; + state = State::after_value; continue; } value += c; break; - case in_value_double_quoted: + case State::in_value_double_quoted: if(c == '"') { - state = after_value; + state = State::after_value; continue; } value += c; break; - case after_value: + case State::after_value: if(std::isspace(c)) { continue; @@ -362,7 +357,7 @@ bool ConfigFile::parseLine(const std::string& line) } } - if(state == before_key) + if(state == State::before_key) { // Line did not contain any data (empty or comment) return true; @@ -370,7 +365,7 @@ bool ConfigFile::parseLine(const std::string& line) // If state == in_value_XXX_quoted here, the string was not terminated. // If state == before_value it means that the value is empty. - if(state != after_value && state != in_value && state != before_value) + if(state != State::after_value && state != State::in_value && state != State::before_value) { ERR(configfile, "Malformed line: '%s'", line.c_str()); return false; diff --git a/src/configfile.h b/src/configfile.h index 0934f46..a2f8db5 100644 --- a/src/configfile.h +++ b/src/configfile.h @@ -33,8 +33,7 @@ class ConfigFile { public: - // TODO: west-const - ConfigFile(std::string const& filename); + ConfigFile(const std::string& filename); virtual ~ConfigFile(); virtual bool load(); @@ -49,6 +48,10 @@ protected: std::fstream current_file; // TODO: Does this have to be virtual? + // Response: This is actually the only member function that has to be virtual. + // I find it very ugly that we have any virtual functions in here, but it + // seems that they were made virtual such that we can test this class properly. + // What do you think? Test differently and make it all non-virtual? virtual bool open(std::ios_base::openmode mode); std::string readLine(); bool parseLine(const std::string& line); |