Page MenuHomeWildfire Games

Remove unused variables and an unused function.
AbandonedPublic

Authored by leper on May 4 2017, 1:11 AM.

Details

Reviewers
Imarok
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

(Partially pointed out by clang warnings.)

Partially because I had parts of that lying around for quite a while.

@Itms it would be nice to have automated builds on some Clang/libc++ system, given that OS X has licensing issues with that we could try with FreeBSD (that might require us to merge a certain patch, but well).

Test Plan

Check that it still builds, or look at the code and see that there are no behavioural changes.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
warning_fixes
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 1420
Build 2237: Vulcan BuildJenkins
Build 2236: arc lint + arc unit

Event Timeline

leper created this revision.May 4 2017, 1:11 AM
Vulcan added a subscriber: Vulcan.May 4 2017, 5:05 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/984/ for more details.

Imarok requested changes to this revision.May 5 2017, 2:21 PM
Imarok added a subscriber: Imarok.

rest looks good

source/ps/XML/Xeromyces.cpp
1

Bump Year

source/soundmanager/scripting/SoundGroup.h
124

I think you should change this comment...

This revision now requires changes to proceed.May 5 2017, 2:21 PM
Imarok added inline comments.May 5 2017, 2:29 PM
source/soundmanager/scripting/SoundGroup.cpp
47

As noticed by bb, this should set to something sane like 3.14159f

vladislavbelov added inline comments.
source/soundmanager/scripting/SoundGroup.cpp
47

Ins't better to have const float PI = ... or even use boost/cmath PI?

Imarok added inline comments.May 5 2017, 2:41 PM
source/soundmanager/scripting/SoundGroup.cpp
47

true.
(but everything is better than defining PI to the wrong number ;)

elexis added a subscriber: elexis.May 5 2017, 2:48 PM
elexis added inline comments.
source/soundmanager/scripting/SoundGroup.h
124

Is it the part that is lying around since a while? :P Actually the comment sounds like it might make sense. Still, out of scope, right?

leper added inline comments.May 8 2017, 4:34 AM
source/soundmanager/scripting/SoundGroup.cpp
47

cmath does not define pi, well it might do, but it isn't required to define it. Most implementations have M_PI because POSIX says that they should. MSVC has it hidden behind some define.

I guess just defining it to something sane as another patch would be nice.

source/soundmanager/scripting/SoundGroup.h
124

Nope, that one was pointed out by clang. Anyone want to figure out what the comment should be changed to?

Imarok added inline comments.May 17 2017, 1:21 AM
source/soundmanager/scripting/SoundGroup.cpp
47

Sounds good.

source/soundmanager/scripting/SoundGroup.h
124

I think as you are removing that var (m_TimeWindow), it should not be used in the comment...
For a more specific suggestion I don't know enough about this file and this vars.

elexis added inline comments.May 17 2017, 1:28 AM
source/soundmanager/scripting/SoundGroup.h
124

Uh, our current intensity (number of sounds played since m_CurTime)? But then again m_Intensity, m_CurTime, potentially more are unused too afaics.

Can we get an update of this (or straight out commit)?

Stan added a subscriber: Stan.Aug 27 2017, 11:31 PM

After discussion with Vladislav today http://irclogs.wildfiregames.com/2017-08-27-QuakeNet-%230ad-dev.log (20:01)

Here is a patch that handles the PI variable. M_PI is handled by precompiled.h

Version without cleanup of the function

Index: source/soundmanager/scripting/SoundGroup.cpp
===================================================================
--- source/soundmanager/scripting/SoundGroup.cpp        (revision 20037)
+++ source/soundmanager/scripting/SoundGroup.cpp        (working copy)
@@ -41,12 +41,8 @@

 #include <algorithm>

-
 extern CGame *g_Game;

-#define PI 3.14126f
-
-
 static const bool DISABLE_INTENSITY = true; // disable for now since it's broken

 void CSoundGroup::SetGain(float gain)
@@ -111,7 +107,7 @@
        float bufferSize = screenWidth * 0.10;
        float yBufferSize = 15;
        const size_t audioWidth = screenWidth;
-       float radianCap = PI / 3;
+       float radianCap = static_cast<float>(M_PI) / 3.f;

        g_Game->GetView()->GetCamera()->GetScreenCoordinates(position, x, y);

Version with cleanup of the function

Index: source/soundmanager/scripting/SoundGroup.cpp
===================================================================
--- source/soundmanager/scripting/SoundGroup.cpp        (revision 20037)
+++ source/soundmanager/scripting/SoundGroup.cpp        (working copy)
@@ -41,12 +41,8 @@
 
 #include <algorithm>
 
-
 extern CGame *g_Game;
 
-#define PI 3.14126f
-
-
 static const bool DISABLE_INTENSITY = true; // disable for now since it's broken
 
 void CSoundGroup::SetGain(float gain)
@@ -105,13 +101,13 @@
 float CSoundGroup::RadiansOffCenter(const CVector3D& position, bool& onScreen, float& itemRollOff)
 {
        float x, y;
-       float answer = 0.0;
+       float answer = 0.0f;
        const size_t screenWidth = g_Game->GetView()->GetCamera()->GetViewPort().m_Width;
        const size_t screenHeight = g_Game->GetView()->GetCamera()->GetViewPort().m_Height;
-       float bufferSize = screenWidth * 0.10;
-       float yBufferSize = 15;
+       float bufferSize = screenWidth * 0.1f;
+       float yBufferSize = 15.f;
        const size_t audioWidth = screenWidth;
-       float radianCap = PI / 3;
+       float radianCap = static_cast<float>(M_PI) / 3.f;
 
        g_Game->GetView()->GetCamera()->GetScreenCoordinates(position, x, y);
 
@@ -131,7 +127,7 @@
        {
                if ((x < 0) || (x > screenWidth))
                {
-                       itemRollOff = 0.5;
+                       itemRollOff = 0.5f;
                }
                float pixPerRadian = audioWidth / (radianCap * 2);
                answer = (x - (screenWidth/2)) / pixPerRadian;
@@ -147,7 +143,7 @@
        }
        else if ((y < 0) || (y > screenHeight))
        {
-               itemRollOff = 0.5;
+               itemRollOff = 0.5f;
        }
 
        return answer;
Stan added inline comments.Aug 27 2017, 11:40 PM
source/soundmanager/scripting/SoundGroup.cpp
47

See patch(es) in above comment.

In D413#32934, @elexis wrote:

Can we get an update of this (or straight out commit)?

Dunno, can I get a suggestion for that one comment everyone seems to be hung up on? I could also just delete that comment.

source/soundmanager/scripting/SoundGroup.cpp
47

[...] another patch [...]

Then again why those casts for M_PI? Also why not 0.f?

Stan added a comment.Sep 6 2017, 6:37 AM

Why not 0.f

Typo

Why cast ?

If question is about static_cast<float> (expr) instead of (float) it's because c style cast are not checked in c++

If it's why there is a cast at all it's because the M_PI defined there has more decimals than a float so I make sure it's one.

elexis added a comment.Sep 6 2017, 7:18 PM
In D413#34236, @leper wrote:

can I get a suggestion for that one comment everyone seems to be hung up on? I could also just delete that comment.

-> https://code.wildfiregames.com/D413#20570
Add m_IntensityThreshold to that list.
The title of the ticket says "remove unused variables" and removes an unused file in SoundGroup.*, so delete the other unused (as in no-read access) variables instead of wondering how they should be described?

The title of the ticket says "remove unused variables"

Maybe I should have used "fix compiler warnings instead", then people would have to complain about compilers not warning about some of those cases.

and removes an unused file in SoundGroup.*, so delete the other unused (as in no-read access) variables instead of wondering how they should be described?

Having done some more digging I'm leaning towards leaving that file alone.

m_Intensity, m_CurTime, m_Priority, m_Decay, m_IntensityThreshold, m_TimeWindow, DISABLE_INTENSITY are never read from. A few of those (Priority, Threshold, Decay) are specified in lots of templates though, and there is #1962 / P41 which sound like they might use some of those.

leper abandoned this revision.Nov 25 2017, 3:55 AM

I'll keep the current thing in my local tree, in case someone cares enough about this to do the work take over.