Fast .slk (SYLK) file parser

Question

For my project I need a (partial) .slk (SYLK) parser which need to load 10+ fairly large tables (30.000-50.000 lines) quickly. The subset is very limited to just the following format:

ID;PWXL;N;E

Always first and it's only important if "ID" is there

B;X79;Y838;D0

The size of the table, always comes before "C" rows. X is the width and Y is the height.

C;X25;K0.25
C;X26;K"always"
C;X26;Y32;K"never"

X is the column and an optional Y sets the row (if missing the last row or 0 is used). The "K" part can either be in quotes or without. Either way the storage format is std::string (for now).

Now for the actual code

std::vector<std::vector<std::string>> table_data;
std::unordered_map<std::string, int> header_to_column;
std::unordered_map<std::string, int> header_to_row;

std::ifstream stream(path);

std::string line;

size_t position = 0;
size_t length = 0;

size_t column = 0;
size_t row = 0;

const auto parse_int_part = [&]() {
    position++;
    length = line.find_first_of(';', position) - position;
    position += length;
    return std::stoi(line.substr(position - length, length));
};

if (std::getline(file, line)) {
    if (line.substr(0, 2) != "ID") {
        std::cout << "Invalid SLK file, does not contain \"ID\" as first record" << std::endl;
        return;
    }
} else {
    return;
}

while (std::getline(stream, line)) {
    position = 2;

    switch (line.front()) {
        case 'B':
            while (true) {
                switch (line[position]) {
                    case 'X':
                        columns = parse_int_part();
                        break;
                    case 'Y':
                        rows = parse_int_part();
                        break;
                    default:
                        table_data.resize(rows, std::vector<std::string>(columns));
                        goto nextline;
                }
                if (position < line.size() - 1) {
                    position++;
                }
            }
        case 'C':
            while (true) {
                switch (line[position]) {
                    case 'X':
                        column = parse_int_part() - 1;
                        break;
                    case 'Y':
                        row = parse_int_part() - 1;
                        break;
                    case 'K': {
                        position++;
                        if (line[position] == '\"') {
                            position++;
                        }

                        length = line.size() - position - ((line.back() == '\r') ? 1 : 0);
                        const std::string part = line.substr(position, length - ((line[position + length - 1] == '\"') ? 1 : 0) );

                        if (row == 0) {
                            header_to_column.emplace(part, column);
                        }

                        if (column == 0) {
                            header_to_row.emplace(part, row);
                        }
                        table_data[row][column] = part;
                        goto nextline;
                    }
                }
                if (position < line.size() - 1) {
                    position++;
                }
            }
        case 'E':
            goto exitloop;
    }
    nextline:
    position = 2;
}
exitloop:;

I use some gotos which are probably ok to use here since the other solutions are a lot more verbose, but are apparently bad practice? And since the file may have "\r\n" line endings those have to be handled manually, but it feels like there should be a better way.

How could the parsing be improved while keeping the same performance?

Edit
I've changed the example so it can be run easily and a simple [test .slk].3.

Answer 1

I gave it a try compiling it on my machine, but there are too many things still missing, so this will be mainly based on visual inspection of the code.

Functions

I would break out various pieces of this into separate named functions. It will greatly improve readability, and likely they'll be small enough that they'll be inlined, anyway, so it probably won't hurt speed. First, reading the header line could be its own function:

bool check_SLK_header(std::fstream file)
{
    bool header_is_good = false;
    std::string line;
    if (std::getline(file, line)) {
        if (line.substr(0, 2) == "ID") {
            header_is_good = true;
        }
    }
    return header_is_good;
}

I would leave printing the error message to the caller of the main function (which you haven't named, so I can't reference it). That separates out the logic of parsing the file from the display of data to the user, which is usually a good thing.

Next I would break out the various cases of the switch statement into their own functions, too. I would name them something like parse_dimensions_record() for the B case, and parse_cell_record() for the C case. I'll talk about the E case below.

Avoid Infinite Loops

This pattern of writing an infinite loop that has some special "break-out" criteria hidden in the middle of it is extremely hard to read and follow. This is one of the problems with using goto, break (outside of a switch statement), continue, etc. It obscures the flow of the code. You can almost always rewrite an infinite loop better with an actual exit criteria. Here's how I'd write the parse_dimensions_record() function for the B case:

void parse_dimensions_record(const std::string& line, std::vector<std::vector<std::string>>& table_data)
{
    int columns = -1;
    int rows = -1;
    size_t position = 2;
    while (((columns < 0) && (rows < 0)) && (position < line.size() - 1))
    {
        switch (line [ position ])
        {
            case 'X':
                columns = parse_int_part();
                break;
            case 'Y':
                rows = parse_int_part();
                break;
        }
        position++;
    }
    
    if ((columns >= 0) && (rows >= 0))
    {
        table_data.resize(rows, std::vector<std::string>(columns));
    }
}

It now becomes clear that there are some edge cases that might warrant handling. First, does it make sense to have 0 columns or 0 rows? If not, you should be returning an error and stop parsing, probably, when you hit that case. If so, then you can also probably end early, as there's no point in reading the rest of the file, as there's no columns or rows to hold any data.

You also don't handle malformed data, such as an invalid specifier in the B record, for example. Is it safe to ignore those? Does it indicate a corrupted file? You probably need to return an error code and handle that case in some way.

I mentioned above that I'd talk about the E case below. Well here we are. The main loop for parsing the file isn't an infinite loop which is good, but you still don't need the goto you have in there. You can end it by just keeping around a variable that says whether you've hit the end record. Something like this:

bool hit_end = false;
while ((std::get line(stream, line)) && (!hit_end)) {
    position = 2;
    
    switch (line.front()) {
        case 'B':
            parse_dimensions_record(line, table_data);
            break;
        case 'C':
            parse_cell_record(line, table_data);
            break;
        case 'E':
            hit_end = true;
            break;
        default:
            // Got some sort of invalid record here. Should probably stop parsing
            break;
    }
}

This is much easier to read and understand and avoids the various goto calls that made the other version difficult to understand.