Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused variables and declarations #30

Merged
merged 1 commit into from
Nov 6, 2014
Merged

Conversation

hach-que
Copy link
Contributor

Mono detects unused variables and private field declarations as warnings. Because ATF has "warnings as errors" enabled, these issues appear as errors and prevent compilation.

This PR removes unused / dead code, unused variables and unused private declarations. In scenarios where the declaration is required (e.g. in one particular Mutex case), I've added a #pragma to disable the warning instead.

@Ron2
Copy link
Member

Ron2 commented Oct 31, 2014

Thank you very much for helping us clean up our code. I'm embarrassed to say that it's been on my task list for a long time now, to use ReSharper to go through all the code systematically and do clean-ups that Visual Studio doesn't help us do by itself.

I've reviewed most of the changes and they all look good so far. I'll get this pull request locally integrated and tested and reviewed. Sometime next week, I should have the changes integrated into master.

Going forward, I wish I had a way to automatically catch these warnings so that we don't accidentally break your project. We could adopt a whole Mono build process, but that might be a lot of maintenance. At the very least we could add a manual step to our major release process (every six months) where we use ReSharper to catch these warnings. If you have any tips, please let me know!

Also, it's exciting that you're using ATF with Mono! Can you tell me anything about your project? Is it on OS X? Using WinForms? Did you have to work around our Win32 dependencies? What about Direct2D?

Thanks again for your improvements.

@hach-que
Copy link
Contributor Author

My plan is to get ATF working on Linux so that the Level Editor etc. can work cross platform. This is so that I can provide a better asset management tool and level editor for http://protogame.org/.

These changes don't actually get it fully building yet as there's some Windows-specific dependencies which don't resolve under Linux.

The next step for me is to clean up and re-organise the projects to support multiple platforms, which I'll file an issue later for so we can discuss the best way of doing so.

@Ron2
Copy link
Member

Ron2 commented Nov 5, 2014

That sounds like a very cool project.

I have a question about this one change to SourceControlRevision.cs:

@@ -77,7 +77,7 @@ public DateTime Date
        {
            get
            {
                if (m_kind != SourceControlRevisionKind.Date || m_dateTime == null) //original line
                if (m_kind != SourceControlRevisionKind.Date) //new line
                    throw new InvalidOperationException("This revision is not a Date");
                return m_dateTime;
            }

The change removes the check for m_dateTime being null. I can see how we should probably be checking this in the setter, but I don't understand how this is a compiler warning. Did you have to remove this check for some other reason?

--Ron

@hach-que
Copy link
Contributor Author

hach-que commented Nov 5, 2014

m_DateTime is of type DateTime, which is a value type and hence can never be null. The compiler was warning about an expression which is always false.

@Ron2
Copy link
Member

Ron2 commented Nov 5, 2014

Ahh, thanks. I'm surprised that ReSharper didn't catch this.

@Ron2 Ron2 merged commit 6d66df7 into SonyWWS:master Nov 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants