Utf-8 check, C++ initial version

[P-D-E]

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

• Have the PR Validation Tests been run? See https://github.com/vegastrike/Vega-Strike-Engine-Source/wiki/Pull-Request-Validation
• This is a documentation change only

Issues:

• Py3 engine segfaults when using a savefile from a py2 engine #149

Purpose:

• What is this pull request trying to do? Preventing crashes from loading non-utf8 saved games via pre-emptive checks, alerting the user when it happens. Serving as a place for discussion of the issue, whatever direction the changes will go.
• What release is this for? 0.8.x, 0.9.x
• Is there a project or milestone we should apply this to? 0.8.x, 0.9.x

[stephengtuggy]

Just a couple of things. Looks good overall. The bit shifting is well documented, which I appreciate.

FWIW, I think savegame.cpp/.h is a perfectly fine place to put this functionality.

Thanks for the PR, @P-D-E !

[royfalk]

A bit late to the party, but I wonder if you could have used some boost facility instead.
Something like https://www.boost.org/doc/libs/1_48_0/libs/locale/doc/html/charset_handling.html. It's possible to ask it to throw an exception. Other than that, LGTM.

[stephengtuggy]

I like this code a lot. I'm ready to give it my approval. What do you guys say?

[BenjamenMeyer]

Generally this looks good; one minor change to move to the smart pointer object and we should be good to go

[BenjamenMeyer]

question: does this need to be a pointer to an instance? Or can it just be a local instance?

Utf8Checker check;
...
if (check.validUtf8(...

If we don't need to allocate, then let's not. If there's a reason we need to, then okay; but I think we may not need to - stack allocation should be sufficient.

NOTE: This wasn't quite clear before.

[P-D-E]

Yes it can, @vegastrike/code-developers do you all agree on this?

[stephengtuggy]

It could just be a local instance instead of a pointer, but personally, I don't think that it matters much one way or the other.

I say let's merge this PR as is.

[ermo]

If the Utf8Checker class instance is only ever used/accessed inside the scope of the isUtf8SaveGame function, then it makes sense to just use it directly as a local instance since it doesn't need to be passed around to other consumers?

[stephengtuggy]

I thought we could save some time by leaving the code the way it is here, but I see that that is not the way this is going. So sure, let's change it to use the UTF8Checker directly, as a local instance.

[BenjamenMeyer]

please port this over to the 0.8.x branch.

[ermo]

please port this over to the 0.8.x branch.

@BenjamenMeyer: Who is supposed to do this?

[BenjamenMeyer]

@ermo ideally the same person that did the PR; but anyone can do it:

$ git checkout 0.8.x
$ git checkout -b port_utf-8-heck
$ git cherry-pick <commit hash 1>
$ git cherry-pick <commit hash ...>
$ git cherry-pick <commit hash N>
$ git push origin port_utf-8-heck

Or something like that...some adjustments might need to be made, but that's the general process

[ermo]

Please port this over to the 0.8.x branch.

Who is supposed to do this?

Ideally the same person that did the PR; but anyone can do it:

$ git checkout 0.8.x
$ git checkout -b port_utf-8-heck
$ git cherry-pick <commit hash 1>
$ git cherry-pick <commit hash ...>
$ git cherry-pick <commit hash N>
$ git push origin port_utf-8-heck

Or something like that...some adjustments might need to be made, but that's the general process

@P-D-E: Would you mind taking a look at porting it to the 0.8.x branch since you're familiar with the code? 🥳

[P-D-E]

@ermo Benjamen and I talked about it and agreed that he'll take care of it 👍