Fixed OpenGL version string being parsed incorrectly on some platforms

[binary1248]

Fixes #1249.

[LaurentGomila]

The string length should be checked in each if, right now you're accessing unchecked elements of the version array, which is UB.

A final else is missing after all the chained ifs, leaving the version number to an undefined state. Maybe we could set it to 1.1 before testing the version, so that we don't have to write else blocks.

[binary1248]

std::strncmp compares two possibly null-terminated character arrays. If the null terminator is encountered in either array before n characters are compared, it ends the comparison. In our case, there is no danger because both parameters are guaranteed to be null-terminated.

[LaurentGomila]

I was talking about what happens inside the blocks:

m_settings.majorVersion = version[13] - '0';
m_settings.minorVersion = version[15] - '0';

strlen(version) should be > 15 in this case.

[binary1248]

I guess you don't want to have to assume that the OpenGL implementation is standard compliant? 😁

[LaurentGomila]

What standard? We are "parsing" strings we know nothing about 😀

[binary1248]

Fixed.

[LaurentGomila]

Looks very robust now 😆

What about a helper function to avoid all these repetitions?

bool parseVersionString(const char* prefix, unsigned int versionPosition, unsigned int &major, unsigned int &minor);

[binary1248]

Naaah... an extra function just to move out 4 if statements? 😨

[mantognini]

It would makes things more readable and shorter. 👍

Maybe even one if statement:

if (parseVersionString(...) || parseVersionString(...) || ...) {
    m_settings.majorVersion = major;
    m_settings.minorVersion = minor;
} else { warning }

with some better formatting, ofc.

[binary1248]

Fixed.

[LaurentGomila]

Is there a reason to use strncmp with the last argument being strlen of one string? If it stops at the first encountered \0 anyway, maybe we can just use strcmp?

[binary1248]

Using std::strncmp we can check for prefixes. std::strncmp("A", "ABC", 1) will return 0 while std::strcmp("A", "ABC") will return non-zero.

[Foaly]

I guess the third parameter could be replaced with versionPosition, but it's a minor detail.

[binary1248]

Fixed.

[eXpl0it3r]

Does it now look okay, @LaurentGomila?

[LaurentGomila]

Yes, it is.

Just for the sake of arguing, it would replace !std::strncmp(...), which feels like "does not compare", with std::strncmp(...) == 0, which looks more intuitive given that the function returns an integer and not a boolean 😬

And of course we should quickly test this new implementation on OpenGL ES platforms to validate it.

[eXpl0it3r]

@kingnobody8 or @JonnyPtn Can you try the code? Need more of an actual test than review. 🙂

[binary1248]

@LaurentGomila Fixed.

[JonnyPtn]

Looks good to me! tested on iPhone 8

[Foaly]

Sorry for the minor remark, but logical ors || make more sense here, don't they?

[JonnyPtn]

I don't think so - it's checking that none of the parse attempts work, not that any of them didn't work

[kingnobody8]

Looks good. Tested on Windows, Android, Mac, and iOS, all successfully.
Just for reference, only the apple versions will do the "old way" of getting the gl version. Windows and android do the "new way".

Issues were resolved