Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Warning cleanup #521

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

sebeksd
Copy link

@sebeksd sebeksd commented May 1, 2016

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.

sebeksd added 22 commits May 1, 2016 18:39
…rided, removed overrided version of Equals and merge it to base class, this commit should be reviewed
…warning (CS0675), this commit should be reviewed
… a debug code, this commit should be reviewed

Additional info:
After using unreachable code space suit color changed from white to red so this code was probably for debugging purposes
… empty statement" (CS0642), this commit should be reviewed
…ed to "GPU" structs or similar "fixed" size types, some was because debug code.

ALSO there are few Sync<bool> (orre similar) or HashSet<> that are null but not sure its correct, should be verified, this commit should be reviewed
…g about hiding parent method (made it virtual)
@Jimmacle
Copy link

Jimmacle commented May 2, 2016

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
Copy link
Author

sebeksd commented May 2, 2016

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
Copy link

Jimmacle commented May 2, 2016

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
Copy link
Contributor

mexmer commented May 2, 2016

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
Copy link
Author

sebeksd commented May 2, 2016

@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
Copy link

devu commented May 2, 2016

@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
Copy link

mze9412 commented May 2, 2016

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
Copy link
Author

sebeksd commented May 2, 2016

@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
Copy link
Member

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
Copy link
Author

sebeksd commented May 3, 2016

@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
Copy link

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
Copy link

ApocDev commented May 14, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants