Vega-strike-engine-source: Enable -Werror compilation

[nabaco]

This is related to #29 and #45.

To enable this, we need to remove all the existing compilation warnings. Once we enable it, the build will fail on any new compilation warnings.
Thus, developers will deal with compilation warnings in real-time, and we'll see the failures in the CI.

[ermo]

Speaking from experience, adding this by default for Release builds (which is what the various distributions and platforms will end up shipping) is _no bueno_.

I would personally prefer if this was only enabled for the Debug target but left off for the other targets.

Then require CI (and thus VS Devs, since CI builds will now gate changes) to build with -DCMAKE_BUILD_TYPE=Debug by default?

[BenjamenMeyer]

@ermo agree - the -Werror is more for us; and good points as far as getting integration into the distros

I do want to _eventually_ get to where there are no errors/warnings, but I don't think we have to necessarily make all warnings errors.

Setting it for our CI pipeline once we get there is a probably a good compromise.

Question: do we want this for the 0.6.x release? Or make it as part of the long term goals?

[ermo]

Question: do we want this for the 0.6.x release? Or make it as part of the long term goals?

My take is "both": Make it a long term goal with a milestone deadline (e.g. 0.9) and then make it a target to get "substantial" work on it done for each preceding milestone (i.e. don't release a milestone unless it contains a subset of this work).

This should get the ball rolling while maintaining forward momentum I should think.

[BenjamenMeyer]

@ermo sounds like a plan. I've been holding off on the 0.9.x target as I've wanted to get at least 0.6.x out first and hadn't identified must specific beyond 0.8.x. This might become the first issue for it. We'll have to think about some related stories to break down the larger work for how to include it into each previous release (0.7.x and 0.8.x)... it might be beyond 0.9.x depending on how much work there is but that'd be okay as long as we're progressing.

[nabaco]

There actually not that much work. Currently the main warnings are:

• Unused variables/functions
• Unsafe string operations
• Class/stracture direct memory access (memset/memcopy)

[BenjamenMeyer]

@nabaco agreed; though some of those might be better broken into smaller parts.

The DMA one at least is probably going to require more dedicated investigation/testing to verify things don't break. IIRC some of those warnings are due to polymorphic behaviors.

The other two should be pretty easy to clean up, as well as any required type casting, etc.

[ermo]

Proposal:

• Do the "easy" clean-ups for 0.6.x, add -Werror to the Debug target
• Begin working on the "hard" cleanup for 0.7.x
• Finish and confirm the "hard" cleanup work (declare it fit for purpose) with 0.8.x or - if necessary - 0.9.x.

[stephengtuggy]

Guys, I think there might be more work here than you're accounting for. At least if you turn on all the warnings as errors.

I say we wait a little longer to tackle this. 0.9.x at the earliest for finishing and confirming the "hard" cleanup work.

[ermo]

Guys, I think there might be more work here than you're accounting for. At least if you turn on all the warnings as errors.

I say we wait a little longer to tackle this. 0.9.x at the earliest for finishing and confirming the "hard" cleanup work.

Given your extensive experience with "chore" refactoring of the code, I'm inclined to take your assessment at face value. =)

Good call.