Page MenuHomeWildfire Games

Removes duplication of Clamp function
AcceptedPublic

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

Details

Reviewers
wraitii
Stan
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 7050
Build 11520: Vulcan BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Stan added inline comments.Jan 24 2019, 11:13 PM
source/graphics/Camera.cpp
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.Jan 24 2019, 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.EditedJan 24 2019, 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.Jan 25 2019, 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.Jan 25 2019, 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.Jan 25 2019, 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.Jan 25 2019, 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.EditedJan 25 2019, 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 :-)

Stan added a subscriber: wraitii.Feb 25 2019, 10:14 AM

Here are some comments while you wait for a good review from @wraitii or someone else.
Maybe you could mark my comments as done so you don't lose track, because you missed some :)

source/graphics/Camera.cpp
228

Missing spaces between operators.

305–306

Missing spaces between operators.

source/graphics/Color.cpp
37

Could have used std::limits but I guess that's too verbose.

source/graphics/HeightMipmap.cpp
107

0.0f

source/graphics/Terrain.cpp
455

Missed a static_cast.

460

Missing static cast.

source/renderer/WaterManager.cpp
1086–1087

Spaces between operators.

source/simulation2/components/CCmpTerritoryManager.cpp
365–366

Pretty sure this isn't returning a U16

source/simulation2/system/TurnManager.cpp
217

missing f / static_casts

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

Why not clamp<u16> ?

71

Why not clamp<u16> ?

Stan requested changes to this revision.Mar 6 2019, 3:11 PM
This revision now requires changes to proceed.Mar 6 2019, 3:11 PM
vladislavbelov marked 13 inline comments as done.

Fixes Stan's notes.

source/graphics/Color.cpp
37

It requires more changes, as adding a component type as own type.

source/simulation2/components/CCmpTerritoryManager.cpp
365–366

It'd be better to fix it separately. Too big patch already.

source/simulation2/system/TurnManager.cpp
217

No need to use static_cast here, all other elements are float.

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

Because you clamping a number more than maximum possible value of u16. If we don't use a bigger type (like int) here, we get an integer overflow.

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/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/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'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1135/display/redirect

Stan added inline comments.Apr 3 2019, 11:42 AM
source/graphics/Camera.cpp
305

Is the compiler smart enough to optimize (mapSize - 1) * TERRAIN_TILE_SIZE) ? as it's also used below ? could be inlined in a variable. Here it's a multiplication while others are mostly substractions

source/graphics/HeightMipmap.cpp
66

Same concern as above.

source/graphics/ObjectBase.cpp
176

Switch case with fallthrough ?

source/renderer/TerrainRenderer.cpp
287

Space between operators :P

source/simulation2/helpers/Render.cpp
91

Why does this one specify Clamp<T> ?

vladislavbelov added inline comments.Apr 3 2019, 1:22 PM
source/graphics/Camera.cpp
305

Yeah, most compilers calculate this expression once.

source/graphics/ObjectBase.cpp
176

Nope, switch accepts only constant expressions.

source/simulation2/helpers/Render.cpp
91

C++ doesn't have number postfix for std::size_t. So I used explicit type to not do explicit static_cast of the constants.

Angen awarded a token.Apr 10 2019, 9:36 PM
Itms added a subscriber: Itms.Apr 10 2019, 9:38 PM
Itms added inline comments.
source/maths/MathUtil.h
26

why did you remove const here?

40

+1 to elexis' remark.

Feels a bit like bike shedding at this point. I suppose this is good to go once the 'else's are fixed.

source/maths/MathUtil.h
26

The syntax looks like pass-by-copy so it may be redundant in most cases, but I suppose we could use pointers as T so I think it'd be worth keeping this const for the sake of clarity.

32

Likewise here.

40

Adding +1 as well because why not.

Stan added inline comments.Apr 17 2019, 6:49 PM
source/maths/MathUtil.h
40

It's been fixed already ^^

Stan resigned from this revision.May 10 2019, 3:10 PM

Not much I can review anymore.

wraitii accepted this revision.May 25 2019, 11:28 AM

fix the const that were removed / sort out what you like for else and commit this.

This revision is now accepted and ready to land.May 25 2019, 11:28 AM