It is never empty so it shouldn't be a pointer.
Likely it takes longer to compile since <functional> is included.
Details
Details
- Reviewers
vladislavbelov wraitii
Start the game nothing should have changed.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped Severity Location Code Message Error source/ps/CStrIntern.cpp:1 LICENCE YEAR1 Inaccurate Copyright Year Error source/ps/CStrIntern.h:1 LICENCE YEAR1 Inaccurate Copyright Year - Unit
Unit Tests Skipped - Build Status
Buildable 22459 Build 54973: Vulcan Build Jenkins Build 54972: Vulcan Build (macOS) Jenkins Build 54971: Vulcan Build (Windows) Jenkins
Event Timeline
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7381/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/8470/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
builderr-debug-gcc7.txt
../../../source/renderer/backend/gl/ShaderProgram.cpp: In constructor 'Renderer::Backend::GL::CShaderProgramGLSL::CShaderProgramGLSL(Renderer::Backend::GL::CDevice*, const CStr8&, const VfsPath&, PS::span<const std::tuple<Path, unsigned int> >, const CShaderDefines&, const std::map<CStrIntern, int>&, int)':
../../../source/renderer/backend/gl/ShaderProgram.cpp:649:31: warning: unused variable 'type' [-Wunused-variable]
for (const auto& [path, type] : shaderStages)
^
builderr-release-gcc7.txt
../../../source/renderer/backend/gl/ShaderProgram.cpp: In constructor 'Renderer::Backend::GL::CShaderProgramGLSL::CShaderProgramGLSL(Renderer::Backend::GL::CDevice*, const CStr8&, const VfsPath&, PS::span<const std::tuple<Path, unsigned int> >, const CShaderDefines&, const std::map<CStrIntern, int>&, int)':
../../../source/renderer/backend/gl/ShaderProgram.cpp:649:31: warning: unused variable 'type' [-Wunused-variable]
for (const auto& [path, type] : shaderStages)
^
In member function 'void CInput::UpdateText(int, int, int)':
cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned int)': specified size 18446744073709551612 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]Link to build: https://jenkins.wildfiregames.com/job/docker-differential/9052/display/redirect
Comment Actions
Not a fan of this. The new syntax is worse, more verbose and it also makes it less obvious that this is actually doing pointer manipulations.
IMO the Pimpl pattern with the m convention, which is common inside the codebase, is clear enough that m always exists. There's basically no risk of messing up here. Thus the syntax overhead doesn't seem warranted.
If we really want a "non-null pointer" wrapper, I'd rather we implement that and overload ->. Would at least make the code readable.
Comment Actions
This doesn't manipulate / mutate the pointer.
IMO the Pimpl pattern with the m convention, which is common inside the codebase, is clear enough that m always exists. There's basically no risk of messing up here. Thus the syntax overhead doesn't seem warranted.
This wasn't the Pimpl pattern.
Comment Actions
It's doing pointer comparison, both equality and ordering, which I'd classify under 'manipulation'.
This wasn't the Pimpl pattern.
It kind of is though? It's not the complete pattern, but it's pretty much the same.
But my point is mostly that this is making the code less readable, even if it theoretically increases type safety. IMO the costs outweigh the benefits fairly clearly here.