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.
Details
- Reviewers
wraitii Stan - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP23002: Removes duplication of Clamp function.
- Apply the patch.
- Compile the game without PCH.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 6841 Build 11245: Vulcan Build Jenkins
Event Timeline
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/
source/maths/MathUtil.h | ||
---|---|---|
41 | I heard we avoid else before return. | |
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 | ||
53 | numeric_limit for that 2^16? |
(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)
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp | ||
---|---|---|
53 | Yeah would be nice. Same below. |
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp | ||
---|---|---|
53 | 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)
Yeah, we discusses about it yesterday with Stan. The problem is that ssize_t is used as size_t in some places.
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 | ||
53 | Why not clamp<u16> ? | |
69 | Why not clamp<u16> ? |
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 | ||
53 | 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
source/graphics/Camera.cpp | ||
---|---|---|
305–306 | 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–67 | 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> ? |
source/graphics/Camera.cpp | ||
---|---|---|
305–306 | 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. |
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. | |
41 | Adding +1 as well because why not. |
source/maths/MathUtil.h | ||
---|---|---|
41 | It's been fixed already ^^ |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/747/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/235/display/redirect
source/maths/MathUtil.h | ||
---|---|---|
32 | I tried to make it more optimized, but it looks more complicated, than someone may expect: #include <type_traits> namespace internal { template <typename T> struct MathArgType { // We check a size of the type to be sure that it might fit in machine // register to pass it by value (currently not all compilers inline // functions with template arguments optimally). using type = typename std::conditional< std::is_arithmetic<T>::value && sizeof(T) <= sizeof(void*), T, const T&>::type; }; template <typename T, typename Arg> inline T Clamp(Arg value, Arg min, Arg max) { if (value <= min) return min; else if (value >= max) return max; return value; } } // namespace internal template <typename T> inline T Clamp(T&& value, T&& min, T&& max) { return internal::Clamp<typename std::decay<T>::type, typename internal::MathArgType<T>::type>(value, min, max); } |