Simple CFG parser in C++

Question

I have written a CFG (text file) parser for my specific needs. I have found other libraries to be much too large. Even other parsers that bill themselves as "simple" run hundreds of line long.

The code that I need reviewed is the parsing and tokenizing of the input. I feel like it can be simplified. I have added more comments than required just to make it easier for you guys to read.

Here are member variables used in the class:

std::string m_cfgFilename;
std::map<std::string, std::string> m_cfgMap;
std::stringstream m_buffer;

The initialization:

bool CFG::Init(std::string filename)
{
    // Save the filename for the reload
    m_cfgFilename = filename;

    if(filename.empty())
    {
        return false;
    }

    std::ifstream t(filename);

    if (!t.is_open())
        return false;

    m_buffer << t.rdbuf();

    Parse();

    return true;
}

The parsing:

void CFG::Parse()
{
    std::string line;
    std::istringstream s(m_buffer.str());
    std::string commentKey("#");

    while (std::getline(s, line))
    {
        // Ignore empty and comment lines
        if (!line.empty() && line.compare(0, commentKey.length(), commentKey))
        {
            // Split the line
            std::vector<std::string> seglist;
            std::string segment;
            std::istringstream ss(line);


            while (std::getline(ss, segment, '='))
            {
                // Trim the white space
                std::stringstream trimmer;
                trimmer << segment;
                segment.clear();
                trimmer >> segment;

                std::transform(segment.begin(), segment.end(), segment.begin(), ::tolower);

                seglist.push_back(segment);
            }

            // Only 
            if (seglist.size() == 2)
            {
                m_cfgMap[seglist[0]] = seglist[1];
            }
        }
    }
}

I am curious if there is a way to cut down on all the different strings and stream conversions.

Here is a sample cfg I would throw at it:

# Test CFG File

# General
name                =   alfred
numPopsicles        =   12
height              =   14.5
loadCharacters      =   true

I should also mention that it does work perfectly. Just worried about redundant code.

Answer 1

Use std::move where appropriate

When you take an object parameter by value with the intent of copying it, use std::move to take advantage of move semantics if the object supports it. This is the case with std::string. But be aware that after moving a string, the original will be left empty, so it is better to restructure CFG::Init to something like the following:

bool CFG::Init(std::string filename)
{
    if (filename.empty())
    {
        return false;
    }

    std::ifstream fileIn(filename);
    if (!fileIn.is_open())
    {
        return false;
    }

    m_buffer << fileIn.rdbuf();

    m_cfgFilename = std::move(filename);
    // 'filename' is now empty. It's contents were take by 'm_cfgFilename'

    Parse();
    return true;
}

It's probably also better to only assign m_cfgFilename at the end, so you don't set that member if you fail to open the file, but that might not be the behaviour you want. Otherwise, you can still move filename to the member string at the top and then use the member string instead.

Also notice that I've renamed the ifstream t in my example. Single letter names are pretty bad, with the exception of a few specific places like in for-loop counters.

CFG::Parse()

This line here is overly complicated:

if (!line.empty() && line.compare(0, commentKey.length(), commentKey))
{
    ...
}

std::string has the == operator overloaded, so you can compare two strings like you would compare a number. But since a comment is any line starting with '#', you might as well just test line[0].

Going further, it's better to avoid deep nesting when you can. Just continue if the condition is true.

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

Better string trim

You can cut down the size of your main reading loop by introducing a small helper function to trim the string. Having such simple utility function at hand can also be useful in other places. You can find a couple nice samples in here. This one in particular might be faster, since it doesn't create the temporary stringstream.

While you are at it, go ahead and turn that to-lowercase transform into a separate function as well:

inline std::string lowercase(std::string str)
{
    std::transform(str.begin(), str.end(), str.begin(), ::tolower);
    return str;
}

A tiny optimization

This is arguably a micro-optimization, but at scale it might have impact. If you move the declaration of your vector seglist and the temporary segment string to the top level of the function, that will avoid recreating the objects at each iteration of the loop. You just have to clear() them at the start of the loop. vector and string might be allocating and freeing dynamic memory on construction/destruction, so if they are only constructed once and just cleared, the memory stays allocated until the end of the function.

The downside of doing this is that you go against the recommendation of declaring variables as close as possible to the first point of usage, so it might not be worth it. Since this is a loop with potential hot code in it, might be interesting.