Page MenuHomeWildfire Games

IGUIObject as an abstract class
Needs ReviewPublic

Authored by elexis on Sat, Aug 3, 6:45 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This patch does the following:

  1. Use override keyword for every IGUIObject function that is overriden, rather than only the one incidence in rP22596
  2. Make the empty HandleMessage function a pure virtual function, so that derived classes don't have to worry about when to call that.
  3. Move AddSetting calls from constructor to AddSettings and call them after the constructor. Required by 2., but also seemingly nice independent cleanup. This is required by 2. because the base class is constructed before the derived class, so every derived IGUIObject would still call the IGUIObject constructor without having overridden the pure virtual HandleMessage function with its implementation in the derived function. As it seems cleaner to define a pure virtual method, require the derived class to implement it, rather than require the derived class to consider when to call the inherited empty constructor that may contain something in the future, I have to propose this patch.
Test Plan

Consider whether this is making it worse or better.
Make sure that the inherited AddSettings calls are at the correct location. This is in particular important for the scrollbar setting and similar ones which are subscribed the most derived class but used in base classes too.
Make sure that what remains in the constructor doesn't end up calling a pure virtual function.
Compile with VS2015, clang and gcc, pass it over to Jenkins to see if none of these compilers has something to complain about.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8715
Build 14276: Vulcan BuildJenkins
Build 14275: arc lint + arc unit

Event Timeline

elexis created this revision.Sat, Aug 3, 6:45 PM
Vulcan added a comment.Sat, Aug 3, 6:51 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/297/display/redirect

elexis edited the test plan for this revision. (Show Details)Sat, Aug 3, 6:56 PM
Vulcan added a comment.Sat, Aug 3, 7:10 PM

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

Linter detected issues:
Executing section Source...

source/gui/CChart.h
|  31| »   std::vector<CVector2D>·m_Points;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::vector' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIObject.h
| 173| »   void·AddChild(IGUIObject*·pChild);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CRadioButton.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUITextOwner.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIButtonBehavior.h
|  49| class·IGUIButtonBehavior·:·virtual·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIButtonBehavior:' is invalid C code. Use --std or --language to configure the language.

source/gui/CProgressBar.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/MiniMap.h
|  28| class·CMiniMap·:·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMiniMap:' is invalid C code. Use --std or --language to configure the language.

source/gui/CInput.h
| 173| »   /**
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

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

source/gui/CSlider.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CImage.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CCheckBox.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CText.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/COList.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIScrollBarOwner.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUIScrollBarVertical.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CDropDown.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CButton.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CTooltip.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/CList.h
| 173| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' 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/298/display/redirect

elexis added a comment.Sat, Aug 3, 7:24 PM

Not sure what Vulcan wants, the first time it had a g++ segfault while building, then I restarted the Vulcan task, then it succeeded.

What it now says is that some code isn't valid c code, but I'm not programming in c, so perhaps it is misconfigured:

[MAJOR] CPPCheckBear (syntaxError):
Code 'std::vector' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIbase.h
57

This screams for a template function.

source/gui/IGUIObject.cpp
151

This function is smoke and mirrors.

If an inherited function has to do some destruction, it must do so in the destructor.

There is no need for this function to exist exept for the only derived class (MiniMap) which uses it to delete textures, and this has no need to use inheritance. It's private logic. The destructor is necessary and sufficient for this task.

wraitii added a subscriber: wraitii.Sun, Aug 4, 9:36 AM

I don't really understand your summary.

Make the empty HandleMessage function a pure virtual function, so that derived classes don't have to worry about when to call that.

Why did they have to worry before? Is the reasoning that there could be something, and now it can't so we're good?

Move AddSetting calls from constructor to... this patch.

I don't really get this whole paragraph, but I must be missing something.

In D2144#89490, @elexis wrote:

Not sure what Vulcan wants, the first time it had a g++ segfault while building, then I restarted the Vulcan task, then it succeeded.
What it now says is that some code isn't valid c code, but I'm not programming in c, so perhaps it is misconfigured:

Yeah, jenkins appears to regularly segfault. And CPPCheckBear needs some configuration to understand that our .h files are C++, not C, which is also broken right now.

source/gui/GUIutil.cpp
303

?