COM_Parse and multiline comments/DP save compatibility

[mh]

Inspired by a thread on Func (clicky), here's a handler for multi-line comments in save games and other files that are parsed by COM_Parse. Pop this after the "skip // comments" section and you can load DarkPlaces save games now.

Code: Select all

   // skip /*..*/ comments
   if (c == '/' && data[1] == '*')
   {
      // comment
      data++;

      while (data[0] && (data[0] != '*' || data[1] != '/'))
         data++;

      if (data[0]) data++;
      if (data[0]) data++;

      goto skipwhite;
   }

[taniwha]

Hmm, I find it hard to imagine that this would cause any compatibility issues, so it's probably a good thing to spread through the various engines.

However, I see a minor bug: "/*/" is treated as a complete comment because you skip over only the opening "/" but not the opening "*".

The double increment after the check for "*/" seemed suspicious, but then I realized it's skipping the closing "*/". I'm not 100% certain (~98%), but I think you can get away with "if (data[0]) data += 2;" because the only way to get there is for data[0] to be 0, or for data[0..1] to be "*/".

[mh]

I was a bit suspicious of the double data++ myself (it's been a coupla years after all) but I've double-checked against the way DarkPlaces handles this and can see that my code was almost a direct copy & paste from it (which I obviously did and then completely forgot about) so I'll leave everything stand.

[taniwha]

Oh, it's correct, but the second "if (data[0])" is redundant (the double increment should be merged).

However, the first single data++ is not correct (doesn't matter what darkplaces does). I've tested

[mh]

I don't disagree. I'm just being wary of the fact that LordHavoc never added a comment explaining his reasons for doing it this way, and am wondering if there was some obscure bug it was intended to fix, or was he just having a brainfart that day? Since the double "if (data[0])" does no harm I'm more inclined to leave it be on account of that.

[taniwha]

I'd say the biggest problem with the double "if (data[0])" is it is confusing.

Any comments on that single "data++" before the while loop?

[mh]

It definitely seems wrong and unnecessary to me too. I can only guess that LH did it because the "handle quoted strings specially" part also does it - but that's all it is, a guess.

Just for reference, here's the relevant section from Q3A:

Code: Select all

      else if (c == '/' && data[1] == '*')
      {
         data += 2;

         while (*data && (*data != '*' || data[1] != '/'))
         {
            data++;
         }

         if (*data)
         {
            data += 2;
         }
      }

I note the use of "data += 2" at the end instead of the double "if (data[0])", but I also note a "data += 2" at the beginning.

[taniwha]

That's very similar to how I implemented it in QF. Differences caused by copying your code and different coding style

Code: Select all

    // skip /*..*/ comments
    if (data[0] == '/' && data[1] == '*') {
        data += 2;      // skip over the leading /*
        while (data[0] && (data[0] != '*' && data[1] != '/'))
            data++;
        if (data[0])
            data +=2;   // skip over the trailing */
        goto skipwhite;
    }


[szo]

The problem with the QF code posted here is that they would stop at the first '*' within the comment, e.g. something like:

Code: Select all

/*
 * $Header$
 */


... will result in '$' as the first available character, an issue which I hit myself. Correct one would be like the following:

Code: Select all

// skip /*..*/ comments
   if (c == '/' && data[1] == '*')
   {
      data += 2;
      while (*data && !(*data == '*' && data[1] == '/'))
         data++;
      if (*data)
         data += 2;
      goto skipwhite;
   }


[taniwha]

Yeah, I got the email, too. I'm considering how to write a test case for "make check".