Warning cleanup

[sebeksd]

I made some warnings cleanup. Some of it was just debug code but few commits (marked in description) should be reviewed because of uncertain changes or even fixes (like Pack64).

Leaved warnings: Obsolate code, user "author" warning/TODOs, processor architecture, also all "Release" warnings about documentation.
At debug now there are around 15 warning, previously there was over 400.
Because solution builds only in 32bits I couldn't carefully check my changes (eg. out of memory).
Moreover newest source code is not building because some missing references.
Most warnings are grouped in single commits.

I hope that my changes will be useful in some way :)

All made in VS 2015.

[Jimmacle]

Did you change random things for the sake of changing them? You didn't just remove warnings (which are there to remind devs of problems or TODOs), you made seemingly completely random modifications most of which don't have any benefit to the code. Sorry, but I highly doubt this will get merged.

[sebeksd]

Why do you think this changes are random ? First of all, 400 warnings are not good as reminder, second, most changes are safe, warnings was there because debug code that was previously commented out. Also there are two fixes (Pack64() and planet distance calculation) in places where warnings appeared.
On the end, it is easier to review this fix (pull request) than do it, and in the worst case just merge part of it. So it's good reason for devs to cleanup code and make their work easier in future.

[Jimmacle]

I only skimmed your changes but here are a few things I noticed:

• Removing usings, which doesn't have any benefit other than saving a couple lines. @OndrejPetrzilka has said that usings shouldn't be touched.
• Removing default parameters. Even if they're never utilized there's no performance gain.
• Using the "new" keyword for members hiding other members. This literally just suppresses the warning, the member should be removed instead *if necessary.
• PR review is significantly more time consuming than doing it internally because all the changes have to be checked to make sure they fit Keen's coding standards and don't do anything the company doesn't want.

[mexmer]

fixes should be posted as separate PR and properly commented, not as part of some huge PR that does other stuff and will take few workdays to review.

other than that, @Jimmacle is correct.

[sebeksd]

@Jimmacle - you say that things I change are not necessary, and yes I agree (expect probable bug fixes), but having tons of warnings also doesn't help.
People that write IDE and compilers (like .NET and VS in this case) known what they do when they add a warning system, so you are using this system taking advantage of it or you just disable it/ ignore it (its not wise).

@Jimmacle and @mexmer - I do not agree that review is so time consuming (of course it's my opinion)

@mexmer - fixes I made are part of removing warnings.

And as a conclusion: I made this changes for devs, if they think they can use some of this stuff (or maybe all of it), I will be happy, if they don't, its fine, in the end this is theirs product, not mine.
So comments like: "it is probably wast of time" - are just a wast of time...

PS:
Working with no warnings increase development time by small amount of time but this approach saves more time that would be wasted on searching errors. In new project I always enable "warning as error" and sometime even increase level of warning.

AND I'm not teaching any one how to develop a software, do not get me wrong.

[devu]

@sebeksd, I totaly agree with you and know when you coming from. Speaking
of waste of time, the warning system is there for a reason. Populating it
with hundred messages ( even if not a big concern ) is both

• Massive waste of time to find the warning you could get writing your part
  of code and can be easily overlooked. Simply this feature is disabled by-
• Not following code standards. Please do not cal (or force others to
  follow) code standards something that is not following them at first place.

This place needs some attitude adjustment because every second thread is
about preventing those who willing to spend their free time to make this
code better. People who for free trying to help getting some angry comments
accusing them for wrong doing. Do you even hear yourself? Try to look at
this from outsider point of view. Step back and ask yourself what is a
purpose of all this?

On 2 May 2016 at 17:42, sebeksd [email protected] wrote:

@Jimmacle https://github.com/Jimmacle - you say that things I change
are not necessary, and yes I agree (expect probable bug fixes), but having
tons of warnings also doesn't help.
People that write IDE and compilers (like .NET and VS in this case) no
what they do when they add a warning system, so you are using this system
taking advantage of it or you just disable it/ ignore it (its not wise).

@Jimmacle https://github.com/Jimmacle and @mexmer
https://github.com/mexmer - I do not agree that review is so time
consuming (of course it's my opinion)

@mexmer https://github.com/mexmer - fixes I made are part of removing
warnings.

And as a conclusion: I made this changes for devs, if they think they can
use some of this stuff (or maybe all of it), I will be happy, if they
don't, its fine, in the end this is theirs product, not mine.
So comments like: "it is probably wast of time" - are just a wast of
time...

PS:
Working with no warnings increase development time by small amount of time
but this approach saves more time that would be wasted on searching errors.
In new project I always enable "warning as error" and sometime even
increase level of warning.

AND I'm not teaching any one how to develop a software, do not get me
wrong.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#521 (comment)

[mze9412]

I am pro fixing warnings but suppressing some by just adding the "new" keyword is not good. If overriding the base class' member/method/property was not intentional then this only hides the problem.

So yes, fixing warnings is good. But be very careful with fixing warnings because it could introduce bugs, especially with things like the "new" keyword.

[sebeksd]

@mze9412 - I agree, and this is why I marked this commit with comment that it needs a review, but still it's just one commit, if "new" is fine for devs (this code was designed this way) they can just merge it, if not they can make it better or just leave, maybe someone will propose better solution
Thanks both @devu and @mze9412 for positive comments :)

[OndrejPetrzilka]

Be careful when fixing "volatile" keyword related warnings. Compiler says it's not treated as volatile sometimes, but removing volatile keyword caused a crash (in Parallel tasks, Fast resource lock or similar places)

[sebeksd]

@OndrejPetrzilka thank you for hint, in terms of these changes there was no "volatile" warnings.
After doing some changes (locally) I'm now able to make more test, and just fixed thing that prevent from saving a game, problem was with one of overrided GetHashCode()

[AlonzoTG]

Yeah, I've been finding that there are a number of booby traps in the code. For example there are some variables that are not used in the code -> warning unused variable... OK.... But well they actually are used and need to be there in order for steam workshop integration to work! So yeah, be very careful....

[ApocDev]

@AlonzoTG if that is the case, then the warning should be suppressed explicitly, not just left as a warning for someone to assume otherwise.