Make CGUISpriteInstance non-copyable to further harden Philips protection from rP1518 against unintentional copies of DrawCall cache such as in rP1507.
Remove copy constructor from rP1536.
Make CGUISpriteInstance movable and use move assignment when sprites are assigned.
Fixes some of the compiler warnings about deprecated copy in rP22443 by not copying instead of copying explicitly.
Add ToJSVal, FromJSVal for CGUISprinteInstance to make JSI_IGUIObject::getProperty and setProperty more consistent.
Rename Sprite operator= to SetName to reduce ambiguity.
Details
Details
- Reviewers
- None
- Commits
- rP22570: CGUISpriteInstance non-copyable and FromJSVal / ToJSVal.
- Trac Tickets
- #5575
Make sure that the removed copies are legitimately leaving the previous variable behind "in some valid but otherwise indeterminate state", for example by noticing that they're not refered to after the moving call.
Diff Detail
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
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
source/gui/CGUISprite.h
| 168|
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.
source/gui/GUIutil.h
| 46| template·<typename·T>
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'template<...' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/262/display/redirect
Comment Actions
Unless perf is a concern, I guess this is likely to be as good as my templated version in P164.
| source/gui/GUIutil.cpp | ||
|---|---|---|
| 329 ↗ | (On Diff #9161) | Would probably be better to get a T&& here |
Comment Actions
The theory says lambda is a std::function function object / functor and thus can be inlined better than a function pointer (irrelevant comparison) and that functions that are defined inside the scope have greater likelihood to be inline than those defined in global scope.
Intuitively it seems like the compiler would have no reason not to inline this one statement.
I compiled using CC="clang -Rpass=inline" CXX="clang++ -Rpass=inline" make -j5, here the output:
A grep for GUIutil.cpp and inline gives 681 results.
Searching specifically for the line questioned:
grep valueSet report -B1 | grep inline
../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUIIbE14SetSettingWrapEP10IGUIObjectRK5CStr8RKbS7_RKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUIIiE14SetSettingWrapEP10IGUIObjectRK5CStr8RKiRKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUIIjE14SetSettingWrapEP10IGUIObjectRK5CStr8RKjRKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUIIfE14SetSettingWrapEP10IGUIObjectRK5CStr8RKfRKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII9CGUIColorE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII11CClientAreaE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII10CGUIStringE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII5CStr8E14SetSettingWrapEP10IGUIObjectRKS0_S5_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII5CStrWE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII6EAlignE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII7EVAlignE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII4CPosE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII8CGUIListE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII10CGUISeriesE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline] ../../../source/gui/GUIutil.cpp:375:2: remark: _ZNKSt8functionIFvvEEclEv inlined into _ZN3GUII18CGUISpriteInstanceE14SetSettingWrapEP10IGUIObjectRK5CStr8RKS0_RKbRKSt8functionIFvvEE with (cost=25, threshold=250) [-Rpass=inline]
If we look carefully, we can find exactly the same types defined in GUItypes.h:
TYPE(bool) TYPE(int) TYPE(uint) TYPE(float) TYPE(CGUIColor) TYPE(CClientArea) TYPE(CGUIString) #ifndef GUITYPE_IGNORE_CGUISpriteInstance TYPE(CGUISpriteInstance) #endif TYPE(CStr) TYPE(CStrW) TYPE(EAlign) TYPE(EVAlign) TYPE(CPos) TYPE(CGUIList) TYPE(CGUISeries)
For gcc one can only enable warning if the compiler decided to ignore an inline keyword, and that can't be defined for functions that are defined in line afaik.
I would assume that inlining a local functor is easy enough for gcc and VS to have it right as well.
| source/gui/CGUI.cpp | ||
|---|---|---|
| 806 ↗ | (On Diff #9161) | std::move for this one too while at it |
Comment Actions
Cheers for actually checking the inline. It does appear you're right, and I don't really have an argument that templates are better here.
When we get if constexpr, that might be the best implementation.