Page MenuHomeWildfire Games

Use variadic template for RecurseObject and move it from GUIutil to IGUIObject
ClosedPublic

Authored by elexis on Aug 20 2019, 3:55 PM.

Details

Summary

1st change: parameter pack
The RecurseObject function currently has four variants, that is four times the code necessary and that only works for four variants, whereas a variadic template works with arbitrary arguments.

2nd change: move to other file
Aside from using parameter pack, the function is also located in GUIutil.h, which is unfortunate, because that file should only contain setting management, but this one is also used for event broadcasting or draw calls.
The function calls a given IGUIObject function on a given IGUIObject and it's children, so it makes more sense to have it in IGUIObject than a historic miscellaneous helper-functions file.

3rd change: CheckRestriction as an argument
The third change in this patch is that the CheckRestriction function and GUIRR enum are superseded with another function pointer to an IGUIObject member.
This way it is cleaner, because the hardcoding is removed, and it is more extensible because IGUIObject inheriting classes can define and pass custom functions.

4th change: drop unused Disabled check

  • The "Disabled" restriction was removed because unused and easily addable for hypothetical users.

5th change: don't iterate base object
There is a TODO in the code asking to not have the base object iterated. Instead of copy&pasting the TODO, it can be deleted or implemented.

Test Plan

Notice that parameter pack is more picky about received arguments and it doesn't seem to work with copying values. That's kind of not bad, because it's nice to avoid copies, especially since those are structs or classes most of the time.
Examine whether std::forward(args)... would do anything.
Examine whether implementing the TODO to not iterate the base-object is useful or counterproductive.
Test the function calls and make sure there is no texture disco thing going on (if you want to see the disco, you have to replace IsHidden with IsInvisible).

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

elexis created this revision.Aug 20 2019, 3:55 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIbase.h
| 162| class·CClientArea
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIObject.h
|  44| template·<typename·T>·class·GUI;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIutil.h
|  35| template<typename·T>·class·GUI;
|    | [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/438/display/redirect

elexis added inline comments.Aug 20 2019, 11:15 PM
source/gui/CGUI.cpp
203 ↗(On Diff #9419)

rather changing function signature

source/gui/IGUIObject.cpp
438 ↗(On Diff #9419)

IsGhost

source/gui/IGUIObject.h
141 ↗(On Diff #9419)

now unused hidden child iterator can be removed. The caller should rather iterate over m_Children or GetChildren() to express what's happening.

Notice that the previous code explicitly converted to T& or const T& due to the previous function signatures of RecurseObject, whereas the new code expects references.
This is mostly due to template deduction: Nothing except Args&& compiles, not Args... args, nor Args&... args, nor Args const&... args, and always with the same complaint about not being able to deduce the correct template.

../../../source/gui/IGUIObject.h:249:7: note: candidate template ignored: deduced conflicting types for parameter 'Args' (<SGUIMessage &> vs. <SGUIMessage>)

Also notice that it may not be Args const&&... because there is at least one function call that mutates the passed argument: m_BaseObject->RecurseObject(nullptr, &IGUIObject::AddToPointersMap, AllObjects).

Examine whether std::forward(args)... would do anything.

Using std::forward seems useless for the current code, because it only choses whether to pick lvalue or rvalue if the called function has both function signatures, but the IGUIObject functions don't consume rvalues, so it never does anything.
Also notice (as mentioned in the stackoverflow link) that using std::forward may actually introduce bugs / unintentional code if the first user of the rvalue consumes the rvalue, then the second user can't use it anymore.
So as far as I see using std::forward or rvalues in the target function would be wrong conceptually.

https://en.cppreference.com/w/cpp/language/parameter_pack
https://en.cppreference.com/w/cpp/language/reference
https://en.cppreference.com/w/cpp/utility/forward
https://stackoverflow.com/questions/7257144/when-to-use-stdforward-to-forward-arguments

source/gui/CGUI.cpp
203 ↗(On Diff #9419)

No I'm not.

255 ↗(On Diff #9419)

Not making this static const, while leaving the ones in IsHidden static const so as not to introduce a regression nor an improvement with that regard.
This should be examined independently and then made consistent globally one way or the other.

source/gui/IGUIObject.cpp
395 ↗(On Diff #9419)

Notice that this change doesn't break conversion from JS::AutoValueVector to JS::HandleValueArray, for example MiniMap.cpp is still happy with it's ScriptEvent("worldclick", paramData); call, the SendEventToAll calls too.

438 ↗(On Diff #9419)

IsHiddenOrGhost, also going public

source/gui/IGUIObject.h
141 ↗(On Diff #9419)

One use in CRadioButton, noticed that GetChildren() doesnt exist yet, adding.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 21 2019, 3:22 PM
This revision was automatically updated to reflect the committed changes.
elexis updated the Trac tickets for this revision.Aug 29 2019, 11:21 AM