Fix parsing files with multiple line continuations

[bprather]

Fixes #936

PR Checklist

• Code passes cpplint
• New features are documented.
• Adds a test for any bugs fixed. Adds tests for new features.
• Code is formatted
• Changes are summarized in CHANGELOG.md
• CI has been triggered on Darwin for performance regression tests.
• Docs build
• (@lanl.gov employees) Update copyright on changed files

[lroberts36]

LGTM, I agree that it might be desirable to re-write this at some point (although I am sure no one will ever have time for that).

[Yurlungur]

I don't fully understand how this change resolves the above issue, but seems like it soils be pretty easy to add a test to catch this, right?

[pgrete]

I now stared at this for some time and also don't fully understand how this fixes the issue.
@bprather , assuming that you've now understood the logic (as you fixed it), would you mind adding additional comments to the existing code so that future us have an easier time to parse the logic (and review)?

[bprather]

I gave it a shot?

I'm honestly not too sure how I could make it clearer or what I could say, sorry. The multiline_x vars are buffers used for line continuations. These buffers were not cleared when treating multiple line continuations (they reside outside the loop over lines in the file), so they accumulated more data over time, every time there was a line continuation. Clearing them fixes the problem.

This is a very common first mistake when writing state-machine parsers (I've made it myself). At risk of sounding sanctimonious, I think this is decent evidence for why we shouldn't be maintaining our own file parser when the problem's been solved well in so many other places.

[pgrete]

I was referring to

    if (continuing || continuation) {   // <--------------------- this logic
      multiline_name += param_name;
      multiline_value += param_value;
      multiline_comment += param_comment;
      continuing = true;
    }
    if (continuing && !continuation) { // <--------------------- and that logic (in combination with what happens inside those statements)
      continuing = false;
      param_name = multiline_name;
      param_value = multiline_value;
      param_comment = multiline_comment;
      // Clear the multiline_x buffers to hold the next multi-line value
      multiline_name = "";
      multiline_value = "";
      multiline_comment = "";
    }

For example, I'm reading if (continuing && !continuation) as "if sth is continuing and at the same time it's not a continuation then..." and cannot really tell what's going on.
Maybe it's just wording, but it seems @Yurlungur also had similar issues parsing the logic in that function.

[bprather]

continuing means we are continuing, in the parser, from a previous line. if (continuing && !continuation) means we've continued from the previous line, but this line does not contain a continuation character. Therefore, it indicates we should flush the current continuation buffer into the line data, because there aren't more lines to read.

The conditionals handle the 2 extra states possible when parsing a line, for a total of 3:

1. Normal line, no continuation
2. Continuation appears at the end of this line, we should append contents to a buffer
3. No continuation appears at the end of this line, we should append contents and then flush the buffer

This is a bog-standard 3-state parser, like CS101 assignment simple, which it looks like Josh threw together very early in Parthenon development. I heavily expect that if either of you spend a few minutes with it, you could understand exactly what's going on, as I did to fix the bug flushing buffers. I do agree the variables could be named better, and I'll add basic comments outlining the states so it can be understood a little more easily.

[Yurlungur]

Thanks @bprather the naming is much clearer now. I know we don't want to put too much effort into this, as it's just a simpler parser, but I do think some unit tests are in order if anyone ever has time.

(Or a rewrite, as @lroberts36 suggests.)

[pgrete]

Thanks @bprather
The updated comments make this indeed much easier to parse (pun intended ;))