ini file parser in C++ - Take 2

Question

Please review my ini file parser (and potentially some general config file formats).

This code is a continuation of this review:

ini file parser in C++

I have tried to incorporate all the suggestions including write to file support. I did not take on the regex suggestion. I didn't really see the need for using it in this application.

config.hpp

#ifndef CONFIG_HPP_
#define CONFIG_HPP_

#include <string>
#include <istream>
#include <unordered_map>

// class to encapsulate ini file style configuration text file
class config
{
public:
    // key=value pair within a section
    typedef std::pair<std::string, std::string> keyvalue;

    // [section1] - top level
    typedef std::unordered_map< std::string, std::unordered_map<std::string, std::string> > sections;

    // construct with a filename
    config(const std::string& filename);
    // construct with a stream
    config(std::istream& strm);
    // no copying allowed
    config(const config&) = delete;
    config& operator = (const config&) = delete;
    // return a copy of whole structure
    const sections get_sections() const;
    // get a value
    const std::string get_value(const std::string& sectionname, const std::string& keyname) const;
    // set a value
    void set_value(const std::string& sectionname, const std::string& keyname, const std::string& value);
    // any changes will not be committed to underlying file until save_changes() is called
    bool save_changes(const std::string& filename);
    // standard way to iterate underlying container
    typedef sections::iterator iterator;
    iterator begin() { return sections_.begin(); }
    iterator end() { return sections_.end(); }

private:
    // parse stream into config data structure
    void parse(std::istream& strm);
    // top level of data structure - hash table
    sections sections_;
};

#endif // CONFIG_HPP_

config.cpp

#include "config.hpp"

#include <iostream>
#include <fstream>
#include <string>
#include <algorithm>
#include <unordered_map>

#include <cstdio>  // std::remove
#include <cassert>


// trim leading white-spaces
static std::string& ltrim(std::string& s) {
    size_t startpos = s.find_first_not_of(" \t\r\n\v\f");
    if (std::string::npos != startpos)  {
        s = s.substr(startpos);
    }
    return s;
}

// trim trailing white-spaces
static std::string& rtrim(std::string& s) {
    size_t endpos = s.find_last_not_of(" \t\r\n\v\f");
    if (std::string::npos != endpos) {
        s = s.substr(0, endpos + 1);
    }
    return s;
}

config::config(const std::string& filename) {
        std::ifstream fstrm;
        fstrm.open(filename);

        if (!fstrm)
            throw std::invalid_argument(filename + " could not be opened");

        parse(fstrm);
}

config::config(std::istream& strm) {
    parse(strm);
}

const config::sections config::get_sections() const {
    return sections_;
}

const std::string config::get_value(const std::string& sectionname, const std::string&keyname) const {

    auto sect = sections_.find(sectionname);
    if (sect != sections_.end()) {
        auto key = sect->second.find(keyname);
        if (key != sect->second.end()) {
            return key->second;
        }
    }

    return "";
}

void config::set_value(const std::string& sectionname, const std::string& keyname, const std::string& value) {

    auto sect = sections_.find(sectionname);

    if (sect == sections_.end()) {
        auto ref = sections_[sectionname];
        sect = sections_.find(sectionname);
    }

    assert(sect != sections_.end());

    auto key = sect->second.find(keyname);
    if (key != sect->second.end()) {
        key->second = value;
    }
    else {
        sect->second.insert(std::make_pair(keyname, value));
    }
}

bool config::save_changes(const std::string& filename) {
    bool success(false);

    // delete existing file - if exists
    std::remove(filename.c_str());
    // iterate thru sections_ saving data to a file
    std::string currentsectionname;
    std::ofstream fstrm;
    fstrm.open(filename);

    if (fstrm.good()) {
        for (auto heading : sections_) {
            fstrm << '[' << heading.first << "]\n";
            for (auto kvs : heading.second) {
                fstrm << kvs.first << '=' << kvs.second << '\n';
            }
            fstrm << '\n';
        }
        success = true;
    }

    return success;
}

void config::parse(std::istream& strm) {

    std::string currentsectionname;
    for (std::string line; std::getline(strm, line);)
    {
        // skip comments
        if (!line.empty() && (line[0] == ';' || line[0] == '#')) {
            // allow both ; and # comments at the start of a line
        }
        else if (line[0] == '[') {
            /* A "[section]" line */
            size_t end = line.find_first_of(']');
            if (end != std::string::npos) {

                // this is a new section so add it to hashtable
                currentsectionname = line.substr(1, end - 1);
                sections_[currentsectionname];
            }
            // else section has no closing ] char - ignore
        }
        else if (!line.empty()) {
            /* Not a comment, must be a name[=:]value pair */
            size_t end = line.find_first_of("=:");
            if (end != std::string::npos) {
                std::string name = line.substr(0, end);
                std::string value = line.substr(end + 1);
                ltrim(rtrim(name));
                ltrim(rtrim(value));

                sections_[currentsectionname].insert(std::make_pair(name, value));
            }
            //else no key value delimitter - ignore
        }
    } // for
}

exercising code.

#include "config.hpp"

#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <algorithm>

void generate_config(const std::string& filename) {
    std::ofstream ostrm;
    ostrm.open(filename);
    if (ostrm) {
        ostrm << "[protocol]\nversion = 6     \n\n[user]\nname = Bob Smith       \nemail = [email protected] \nactive = true\n\npi = 3.14159";
    }
}


int main() {

    // generate test file
    generate_config("test1.ini");

    // retrieve some information from config file
    try {
        config cfg("test1.ini");

        std::cout << "email=" << cfg.get_value("user", "email") << '\n';

        // iterate sections in ini file
        for (auto& sec : cfg) {
            std::cout << "section name: " << sec.first << std::endl;
            // iterate key value pairs in section
            for (auto& data : sec.second) {
                std::cout << "key: " << data.first << ", value: " << data.second << std::endl;
            }
        }

        const config::sections configs = cfg.get_sections();
        for (auto heading : configs) {
            std::cout << "heading: " << heading.first << std::endl;
            for (config::keyvalue kvs : heading.second) {
                std::cout << "first: " << kvs.first << " second: " << kvs.second << std::endl;
            }
        }

        // now make a change and save changes
        cfg.set_value("user", "colour", "red");

        bool saved = cfg.save_changes("test1.ini");

        std::cout << (saved ? "file successfully updated\n" : "error saving changes to file\n");

        // construct with a stream test
        std::stringstream ss{ "[protocol]\nversion = 6     \n\n[user]\nname = Bob Smith       \n"
            "email = [email protected] \nactive = true\n\npi = 3.14159" };
        config cfg2(ss);
        // now save as a persistent file
        cfg2.save_changes("stuff.ini");

        config cfg3("stuff.ini");
        const std::string ver = cfg3.get_value("protocol", "version");
        std::cout << "construct with stream test " << (ver == "6" ? "passed" : "failed") << std::endl;

    }
    catch (std::exception& e) {
        std::cout << "Exception occurred: " << e.what() << std::endl;
    }
}

Answer 1

This version seems much improved. Good job! There are still a few things that might be further improved, which I list below.

Avoid defining types that aren't used

The config.hpp code currently contains this line:

typedef std::pair<std::string, std::string> keyvalue;

However, that type isn't actually used anywhere within the header. It's only used within the implementation once where it could, in fact, have been replaced with auto.

Combine declaration and initialization

In a few places in the code where a std::ofstream is used, we have code like this:

std::ofstream fstrm;
fstrm.open(filename);

It's generally better in C++ to initialize variables immediately because this reduces the possibility that uninitialized variables will inadvertently be used. It also has the side effect of making the code shorter and, in my view, easier to read and understand. That might look like like this instead:

std::ofstream fstrm{filename};

Note that I have used the C++11 style initialization syntax here. If your compiler does not support C++11, I'd recommend either updating/replacing the compiler to one that does or falling back to the C++03 syntax using ().

Check function return values

Within the save_changes() function, the code has this line:

// delete existing file - if exists
std::remove(filename.c_str());

However, the return code is not consulted. It's possible the this call could fail, as with a read only file or a file that doesn't exist. I'd recommend either checking the return value or using the following suggestion instead.

Use file modes effectively

One way to simplify the save_changes() code would be to remove the separate file delete and then write, and simply use the appropriate file mode instead:

std::ofstream fstrm{filename, std::ios_base::out | std::ios_base::trunc};

Avoid making copies when practical

The save_changes() code includes these loops:

for (auto heading : sections_) {
    fstrm << '[' << heading.first << "]\n";
    for (auto kvs : heading.second) {
        fstrm << kvs.first << '=' << kvs.second << '\n';
    }

To let the compiler know that you don't need copies because the underlying objects will not be modified, I'd recommend writing it this way instead:

for (const auto &heading : sections_) {
    fstrm << '[' << heading.first << "]\n";
    for (const auto &kvs : heading.second) {
        fstrm << kvs.first << '=' << kvs.second << '\n';
    }

Use const where practical

Most places that could use const seem to be doing so, which is good. I'd suggest that the save_changes() function could also be const.

Eliminate unused variables

The variable currentsectionname in your save_changes code is defined but never used. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Make early bailout explicit and obvious

In the save_changes code, if the file fails to open, the function returns false. This could be made much more clear and explicit. Here's how I'd rewrite that function using a number of the suggestions above:

bool config::save_changes(const std::string& filename) const {
    std::ofstream fstrm{filename, std::ios_base::out | std::ios_base::trunc};

    if (!fstrm) {
        return false;
    }
    for (const auto &heading : sections_) {
        fstrm << '[' << heading.first << "]\n";
        for (const auto &kvs : heading.second) {
            fstrm << kvs.first << '=' << kvs.second << '\n';
        }
        fstrm << '\n';
    }
    return true;
}

Simplify parsing logic

The first part of the compound if statement within the parse() routine is this:

// skip comments
if (!line.empty() && (line[0] == ';' || line[0] == '#')) {
    // allow both ; and # comments at the start of a line
}

However, since empty lines can also be skipped, this could instead be written as:

if (line.empty() || line[0] == ';' || line[0] == '#') {

Due to short circuit evaluation, the line[0] comparisons will only be invoked if the the line is not empty, which is exactly what you want here. It also has the pleasant side effect of changing the last else from this:

else if (!line.empty()) {

To this:

else {

Consider using raw character strings

With C++11, we have the option of using raw character strings. This may make things such as the generate_config a little easier to read:

void generate_config(const std::string& filename) {
    std::ofstream{filename} <<
    R"([protocol]
version = 6

[user]
name = Bob Smith
email = [email protected]
active = true

pi = 3.14159)";

}

Prefer named values to literal strings

In the ltrim and rtrim routines, both use the same literal string " \t\r\n\v\f". I would suggest making that a named static const string such as whitespace.

Answer 2

Simplify set

void config::set_value(const std::string& sectionname, const std::string& keyname, const std::string& value) {
    sections_[sectionname][keyname] = value;
}

Overwriting a file has the same affect as removing it:

// delete existing file - if exists
std::remove(filename.c_str());
// iterate thru sections_ saving data to a file
std::string currentsectionname;
std::ofstream fstrm;
fstrm.open(filename);

Can be simplified too:

std::ofstream fstrm(filename);

Answer 3

About your trim functions:

You want to erase the whitespace so use the appropriate function http://www.cplusplus.com/reference/string/string/erase/

// trim trailing white-spaces
static std::string& rtrim(std::string& s) {
    size_t endpos = s.find_last_not_of(" \t\r\n\v\f");
    if (std::string::npos != endpos) {
        s.erase(endpos, std::string::npos);
    }
    return s;
}

This has a second advantag, now when endpos == std::string::npos nothing will happen, so we can avoid the if

// trim trailing white-spaces
static std::string& rtrim(std::string& s) {
    size_t endpos = s.find_last_not_of(" \t\r\n\v\f");
    s.erase(endpos, std::string::npos);
    return s;
}

Now we only use the endpos once so inline it

// trim trailing white-spaces
static std::string& rtrim(std::string& s) {
    s.erase(s.find_last_not_of(" \t\r\n\v\f"), std::string::npos);
    return s;
}

Now the return is really not needed,and a look at the reference tells us that erase returns a string& http://www.cplusplus.com/reference/string/string/erase/

// trim trailing white-spaces
static std::string& rtrim(std::string& s) {
    return s.erase(s.find_last_not_of(" \t\r\n\v\f"), std::string::npos);
}

Actually there is a std::isspace for your characters, however find_last_of does not seem to take a predicate, so we are stuck with a constant.

What is also really useful is the deletion of multiple consecutive whitespaces. Here std::unique is your friend

// Remove consecutive white-spaces
static std::string& cleanWhitespace(std::string& s) {
    auto end = std::unique(s.begin(), s.end(), [](const char& l, const char& r) {
        return std::isspace(l) && l == r;
    });
    s.erase(end, s.end());
    return s;
}

Sadly s.erase only returns the iterator so we need the separate return