HomeWildfire Games

Implement IGUIObject settings as a template class and IGUIObject::AddSetting as…
Audit RequiredrP22604

Description

Implement IGUIObject settings as a template class and IGUIObject::AddSetting as a template function.

This means the type information is available for all methods operating with the setting type, which is easier for the authors, allows for compile-time checks and optimizations.
Remove the enum that was used to indirectly obtain the setting type at runtime.
Revises SGUISetting and enum from rP290 (rP74), std::function from rP22574.

Differential Revision: https://code.wildfiregames.com/D2145
Tested on: gcc, clang, VS2015, Jenkins

Event Timeline

elexis added a comment.Aug 5 2019, 4:22 AM

09:30 < Angen> i recall gameboy reported something about strange colour in forum too
09:30 < Angen> https://wildfiregames.com/forum/index.php?/topic/26694-strange-font-color/

After spending the entire day watching the compiler in a VM and it freezing my system, killing apps and becoming familiar with commandline building on VS2015 and chasing mysteries, I have found two findings and remedies.

Firstly it was this commit that has shown the symptom of hovered button text being black instead of white.
But I don't find a bug in this commit.

What I do find is that if the CGUIColor class from rP22558 doesn't use the using-declaration but creates custom overriding constructors calling the inherited constructor, the issue is gone;
or alternatively, if the member initialization list of CGUISetting is changed from m_pSetting(T()) to m_pSetting = T();, it works too (without removing the using declaration).

https://en.cppreference.com/w/cpp/language/using_declaration

using CColor::CColor;

using declaration is a C++17 feature, so it shouldn't be used to begin with.
I will try to force C++11 dialect for compiling, perhaps Jenkins can do it too, so that we don't unintentionally use too recent features.

It still strikes me as odd that this is bugged on VS2015, because they claim to support it:
https://docs.microsoft.com/en-us/cpp/cpp/using-declaration?view=vs-2015

And CGUIColor() and new CGUIColor() also return the correct values.

So it seems that there is a VS2015 bug when a member init list uses a using-declarated constructor.

However I could not replicate this issue with a minimal class example.

So I suppose it also plays a role that the two classes are in different translation units.

But it certainly seems like a VS bug, because the constructors are called, and they are called according to the specs, and only VS is not seeming to implement them correctly.

elexis added a comment.Aug 5 2019, 9:29 PM

This is the C++17 removing diff and thus seems obvious to commit. It fixes the black color issue. As mentioned VS2015 claims to support it however.

Index: source/gui/CGUIColor.h
===================================================================
--- source/gui/CGUIColor.h	(revision 22615)
+++ source/gui/CGUIColor.h	(working copy)
@@ -26,8 +26,16 @@
  */
 struct CGUIColor : public CColor
 {
-	 using CColor::CColor;
+	 CGUIColor()
+	 {
+		 CColor();
+	 }
 
+	 CGUIColor(float r, float g, float b, float a)
+	 {
+		 CColor(r, g, b, a);
+	 }
+
 	 bool ParseString(const CStr& value, int defaultAlpha = 255)
 	 {
 		 return g_GUI->GetPreDefinedColor(value, *this) || CColor::ParseString(value, defaultAlpha);

However when I tried the same code in a separate class I could not reproduce the issue.
So I wonder whether such a commit would hide platform dependent undefined behavior somewhere else.

Index: source/gui/GUIutil.cpp
===================================================================
--- source/gui/GUIutil.cpp	(revision 22615)
+++ source/gui/GUIutil.cpp	(working copy)
@@ -25,8 +25,9 @@
 
 template<typename T>
 CGUISetting<T>::CGUISetting(IGUIObject& pObject, const CStr& Name)
-	: m_pSetting(T()), m_Name(Name), m_pObject(pObject)
+	: m_Name(Name), m_pObject(pObject)
 {
+	m_pSetting = T();
 }
 
 template<typename T>

This is the other diff that I found "experimentally" that also removes the black-font symptom.

I could neither reproduce this bug in separate code.

I tried this way for instance:

Index: source/gui/CButton.cpp
===================================================================
--- source/gui/CButton.cpp	(revision 22616)
+++ source/gui/CButton.cpp	(working copy)
@@ -21,7 +21,29 @@
 
 #include "gui/CGUIColor.h"
 #include "lib/ogl.h"
+#include "ps/CLogger.h"
+#include "gui/File2.h"
 
+struct class1 {
+
+	class1()
+	: i(-5)
+	{
+	}
+
+	int i;
+};
+
+struct class2 : class1 {
+	using class1::class1;
+};
+
+template<typename T>
+struct container {
+	container() : cl2(T()) {}
+	T cl2;
+};
+
 CButton::CButton(CGUI* pGUI)
 	: IGUIObject(pGUI), IGUIButtonBehavior(pGUI), IGUITextOwner(pGUI)
 {
@@ -47,6 +69,10 @@
 	AddSetting<CStrW>("tooltip");
 	AddSetting<CStr>("tooltip_style");
 
+	//container<class2> cont;
+	Class2 cl2;
+	LOGERROR("cl2.i is %d", cl2.member);
+
 	// Add text
 	AddText(new SGUIText());
 }

But it doesn't reproduce the bug.

Also ENSURE(CGUIColor() == CGUIColor()); is always true (with and without the patch).
And CGUIColor() and new CGUIColor return the intended values also (with and without the patch).

Just when used here in deployment, it bugs for some reason.

The right thing would be to have some more eyes on this, if there was a Windows Developers Team © ®, they had the chance to identify a bug in VS 2015 and perhaps report upstream, or show a camouflaged undefined behavior in this patch.

This commit now requires audit.Aug 5 2019, 9:45 PM
FeXoR added a subscriber: FeXoR.Aug 8 2019, 10:40 AM

I agree adding your "C++17 removing diff" would be the way to go here.
If you already created a Phabricator differential a link to that here would be nice.

elexis added inline comments.Aug 26 2019, 3:21 PM
/ps/trunk/source/gui/IGUIObject.h
35

<functional> from rP22574 obsolete as of this commit