Page MenuHomeWildfire Games

Removes duplication of Clamp function
Needs ReviewPublic

Authored by vladislavbelov on Thu, Jan 24, 9:52 PM.

Details

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

There was 2 similar implementation of the Clamp function. I left the MathUtil implementation, because I think the function is about math abstraction, not about low level operations.

Test Plan
  1. Apply the patch.
  2. Compile the game without PCH.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6845
Build 11249: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.Thu, Jan 24, 9:52 PM
Stan added a subscriber: Stan.Thu, Jan 24, 10:01 PM
Stan added inline comments.
source/graphics/Terrain.cpp
121–122

static_cast here and below

133–134

same as above

239–240

static cast here and below

311–312

static_cast

318–319

static_cast

327–328

static_cast

346–347

static_cast

int should probably be ssize_t

351–352

static_cast

409–410

static_cast

454–455

static_cast int → ssize_t

460

static_cast

484

static_cast

655

static_cast

675

static_cast

703

static_cast

source/gui/IGUIScrollBar.cpp
61

static_cast

source/ps/CConsole.cpp
387

static_cast

source/renderer/TerrainOverlay.cpp
138

static_cast

source/renderer/TexturedLineRData.cpp
395

static_cast

source/renderer/WaterManager.cpp
780–781

missing spaces between operators.

1080

missing spaces.

1086–1087

static_cast

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
55

static_cast

71

static_cast

84

static_cast

420

static_cast

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
176–177

static_cast

187–188

static_cast

Stan added a comment.Thu, Jan 24, 10:02 PM

Which of the two implementation was the fastest ? :)

Vulcan added a subscriber: Vulcan.Thu, Jan 24, 10:19 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/999/

vladislavbelov added a comment.EditedThu, Jan 24, 10:23 PM
In D1763#70987, @Stan wrote:

Which of the two implementation was the fastest ? :)

This one is faster for me:

template <typename T>
inline T Clamp(T value, T min, T max)
{
	ASSERT(min <= max);
	if (value < min)
		return min;
	else if (value > max)
		return max;
	else
		return value;
}

I suppose, because min/max can be not inlineable functions for MVSC.

Stan added a comment.Thu, Jan 24, 10:55 PM

https://pastebin.com/eGu9M6Z5

Mingw32

526867 526867
967128 967128
Time1: 93.267ms
Time2: 75.103ms
526867 526867
967128 967128
Time1: 93.267s
Time2: 75.103s
mingw32-g++ fine.cpp -Wall -Wextra -Wpedantic  -O3
526867 526867
967128 967128
Time1: 9.841s
Time2: 5.476s
526867 526867
967128 967128
Time1: 15.114ms
Time2: 4.973ms
mingw32-g++ fine.cpp -Wall -Wextra -Wpedantic  -O9
526867 526867
967128 967128
Time1: 9.345ms
Time2: 3.337ms
526867 526867
967128 967128
Time1: 10.002ms
Time2: 3.737ms

MSVC 2015

cl.exe fine.cpp /EHsc
220854 220854
925362 925362
Time1: 70.046s
Time2: 55.939s
220854 220854
925362 925362
Time1: 72.240s
Time2: 54.569mss
cl.exe fine.cpp /EHsc /O2
220854 220854
925362 925362
Time1: 4.641ms
Time2: 3.380ms
220854 220854
925362 925362
Time1: 6.060ms
Time2: 2.970ms
cl.exe fine.cpp /EHsc /Ox
220854 220854
925362 925362
Time1: 3.941ms
Time2: 3.130ms
220854 220854
925362 925362
Time1: 3.923ms
Time2: 3.139ms

Fixes Stan notes.

Stan added a comment.Thu, Jan 24, 11:13 PM

Still some missing :P

source/graphics/Camera.cpp
227–228

static_cast

305–306

static_cast

source/graphics/Color.cpp
37

rgb are ints ?

source/graphics/Decal.cpp
50

static_cast

source/graphics/HFTracer.cpp
327

static_cast

source/graphics/HeightMipmap.cpp
67

static_cast

109

static_cast

124–125

static_cast

125

static_cast

source/graphics/Terrain.cpp
242–243

spaces

413

static_cast spaces

source/renderer/TexturedLineRData.cpp
395

spaces

source/simulation2/helpers/Pathfinding.h
148–149

static_cast

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1000/

vladislavbelov marked an inline comment as done.

Fixes another Stan notes.

source/graphics/Color.cpp
37

SColor4ub - yes.

Stan added inline comments.Thu, Jan 24, 11:44 PM
source/graphics/HeightMipmap.cpp
109

Still missing :P

125

Here as well :)

source/graphics/ParticleEmitterType.cpp
590–591

static_cast, spaces

source/graphics/Terrain.cpp
346–347

Not done :)

351–352

Here too :D

413

spaces

Fixes other another Stan notes.

Stan added a comment.EditedThu, Jan 24, 11:59 PM

Builds in debug and release configurations on windows without pch.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1001/

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1002/

elexis added a subscriber: elexis.Fri, Jan 25, 2:53 PM
elexis added inline comments.
source/maths/MathUtil.h
40

I heard we avoid else before return.
(If there is code, people think it has a use, but these elses don't. Secondly it costs a bit of time to read characters)

source/simulation2/components/CCmpPosition.cpp
789
source/simulation2/helpers/Render.cpp
91

(I read people asked about static casts?)

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
55

numeric_limit for that 2^16?

smiley added a subscriber: smiley.Fri, Jan 25, 3:04 PM

(One can argue that something worse than an inconsistent code base is inconsistency within a single file. But then again, such arguments are probably what led to this situation in the first place)

Stan added inline comments.Fri, Jan 25, 8:03 PM
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
55

Yeah would be nice. Same below.

vladislavbelov marked 2 inline comments as done.

Fixes elexis and another other another Stan notes.

source/simulation2/components/CCmpPosition.cpp
789

Hmm...

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
55

It'd be good if you test that the std::numeric_limits<u16>::max() is correct for you.

Stan added inline comments.Fri, Jan 25, 9:07 PM
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
55

in source\lib\types.h

typedef uint8_t u8;
typedef uint16_t u16;
typedef uint32_t u32;
typedef uint64_t u64;

MSVC

c:\Users\dolci\Desktop>cl.exe fine.cpp /EHsc

c:\Users\dolci\Desktop>fine.exe
std::numeric_limits<u16>::max(): 65535

MINGW

c:\Users\dolci\Desktop>mingw32-g++ fine.cpp

c:\Users\dolci\Desktop>a.exe
std::numeric_limits<u16>::max(): 65535

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1003/

(Re: ssize_t to size_t
I thought that this was discouraged since it can introduce subtle bugs.
Google's C++ style guide also mentions something like this)

In D1763#71069, @smiley wrote:

(Re: ssize_t to size_t
I thought that this was discouraged since it can introduce subtle bugs.
Google's C++ style guide also mentions something like this)

Yeah, we discusses about it yesterday with Stan. The problem is that ssize_t is used as size_t in some places.

smiley added a comment.EditedFri, Jan 25, 9:45 PM
In D1763#71069, @smiley wrote:

(Re: ssize_t to size_t
I thought that this was discouraged since it can introduce subtle bugs.
Google's C++ style guide also mentions something like this)

Yeah, we discusses about it yesterday with Stan. The problem is that ssize_t is used as size_t in some places.

Just pointing it out. You know way more about C++ than me to make the right call :-)