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
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Fixes another Stan notes.
source/graphics/Color.cpp | ||
---|---|---|
37 ↗ | (On Diff #7390) | SColor4ub - yes. |
source/graphics/HeightMipmap.cpp | ||
---|---|---|
109 ↗ | (On Diff #7390) | Still missing :P |
125 ↗ | (On Diff #7390) | Here as well :) |
source/graphics/ParticleEmitterType.cpp | ||
591 ↗ | (On Diff #7391) | static_cast, spaces |
source/graphics/Terrain.cpp | ||
413 ↗ | (On Diff #7390) | spaces |
346 ↗ | (On Diff #7389) | Not done :) |
351 ↗ | (On Diff #7389) | Here too :D |
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 ↗ | (On Diff #7392) | I heard we avoid else before return. |
source/simulation2/components/CCmpPosition.cpp | ||
789 ↗ | (On Diff #7392) | |
source/simulation2/helpers/Render.cpp | ||
91 ↗ | (On Diff #7392) | (I read people asked about static casts?) |
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp | ||
53 ↗ | (On Diff #7389) | 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 ↗ | (On Diff #7389) | Yeah would be nice. Same below. |
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp | ||
---|---|---|
53 ↗ | (On Diff #7389) | 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 | ||
---|---|---|
305 ↗ | (On Diff #7390) | Missing spaces between operators. |
228 ↗ | (On Diff #7395) | Missing spaces between operators. |
source/graphics/Color.cpp | ||
37 ↗ | (On Diff #7390) | Could have used std::limits but I guess that's too verbose. |
source/graphics/HeightMipmap.cpp | ||
107 ↗ | (On Diff #7395) | 0.0f |
source/graphics/Terrain.cpp | ||
455 ↗ | (On Diff #7395) | Missed a static_cast. |
460 ↗ | (On Diff #7389) | Missing static cast. |
source/renderer/WaterManager.cpp | ||
1086 ↗ | (On Diff #7389) | Spaces between operators. |
source/simulation2/components/CCmpTerritoryManager.cpp | ||
365 ↗ | (On Diff #7395) | Pretty sure this isn't returning a U16 |
source/simulation2/system/TurnManager.cpp | ||
217 ↗ | (On Diff #7395) | missing f / static_casts |
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp | ||
53 ↗ | (On Diff #7389) | Why not clamp<u16> ? |
69 ↗ | (On Diff #7389) | Why not clamp<u16> ? |
Fixes Stan's notes.
source/graphics/Color.cpp | ||
---|---|---|
37 ↗ | (On Diff #7390) | It requires more changes, as adding a component type as own type. |
source/simulation2/components/CCmpTerritoryManager.cpp | ||
365 ↗ | (On Diff #7395) | It'd be better to fix it separately. Too big patch already. |
source/simulation2/system/TurnManager.cpp | ||
217 ↗ | (On Diff #7395) | No need to use static_cast here, all other elements are float. |
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp | ||
53 ↗ | (On Diff #7389) | 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 ↗ | (On Diff #7619) | 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 ↗ | (On Diff #7619) | Same concern as above. |
source/graphics/ObjectBase.cpp | ||
176 ↗ | (On Diff #7619) | Switch case with fallthrough ? |
source/renderer/TerrainRenderer.cpp | ||
287 ↗ | (On Diff #7619) | Space between operators :P |
source/simulation2/helpers/Render.cpp | ||
91 ↗ | (On Diff #7392) | Why does this one specify Clamp<T> ? |
source/graphics/Camera.cpp | ||
---|---|---|
305 ↗ | (On Diff #7619) | Yeah, most compilers calculate this expression once. |
source/graphics/ObjectBase.cpp | ||
176 ↗ | (On Diff #7619) | Nope, switch accepts only constant expressions. |
source/simulation2/helpers/Render.cpp | ||
91 ↗ | (On Diff #7392) | 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 ↗ | (On Diff #7619) | 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 ↗ | (On Diff #7619) | Likewise here. |
40 ↗ | (On Diff #7619) | Adding +1 as well because why not. |
source/maths/MathUtil.h | ||
---|---|---|
40 ↗ | (On Diff #7619) | 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 ↗ | (On Diff #7619) | 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); } |