Page MenuHomeWildfire Games

Move CGUI::GenerateText to CGUIText ctor and split into helper functions
ClosedPublic

Authored by elexis on Aug 11 2019, 4:01 PM.

Details

Summary

The CGUI::GenerateText function:

  • consists of 300 lines of code, so it's quite long. It has deep and intransparent nesting. So it would be better split into helpers.
  • Does nothing other than creating a SGUIText struct, so it is a non-default constructor. Moving it over also means the code is better sorted, and CGUI becomes more agnostic of how text is handled by the GUI.
  • Making this a constructor also means the IGUITextOwner may construct the texts in place instead of having to copy or move assign them over to its map / vector.
  • Being able to construct the elements in place also means the IGUITextOwner can work with references instead of pointers without adding a performance regression, allowing us to remove delete calls without remorse.
  • Rename SGUIText to CGUIText and GUItext.h to CGUIText.h, making this filename more consistent with the rest of the folder.
  • Uses two std::array instead of [2], and CSize line_size instead of line_width and line_height

The purpose of the diff is to address these two things without introducing any behavior changes.

Revision history:
rP290 introduced GenerateText to CGUI.
rP22637 introduced more move semantics for the GUI.
rP22643 moved CGUIText to a custom header, allowing this code to be introduced in a single cpp file.

Test Plan

Compile with clang reporting inlines, to ensure that the helper functions are getting inlined.
Compare the previous code against the new code to ensure that the code is the same.

To test clang inline output, first do a clean build without output:

CC="clang" CXX="clang++" make -j4

Then update the header file and build with the report setting enabled to only get the report for the more relevant code.

CC="clang -Rpass=inline" CXX="clang++ -Rpass=inline" make -j3

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 11 2019, 4:01 PM

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

Linter detected issues:
Executing section Source...

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

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/IGUITextOwner.h
|  23| »   Interface·class·that·enhance·the·IGUIObject·with
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIList{' is invalid C code. Use --std or --language to configure the language.

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

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

source/gui/GUI.h
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/gui/GUI.h
|  23| »   Include·this·file·and·it·will·include·the·whole·GUI.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIList{' 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/353/display/redirect

elexis edited the summary of this revision. (Show Details)Aug 11 2019, 4:11 PM
elexis added a comment.EditedAug 11 2019, 6:10 PM

Found this with Clang, but not gcc / Jenkins:

../../../source/gui/CGUIText.h:48:1: warning: 'CGUIText' defined as a class here but previously declared as a struct; this is valid, but may result in linker
      errors under the Microsoft C++ ABI [-Wmismatched-tags]
class CGUIText
^
../../../source/gui/CGUI.h:57:1: note: did you mean class here?
struct CGUIText;
^~~~~~
class

To report whether things were inlined:

CC="clang -Rpass=inline" CXX="clang++ -Rpass=inline" make -j3

One can search that massiv output using grep 'CGUIText.cpp:178' where 178 is the line number.

  • ComputeLineRange is inlined:

../../../source/gui/CGUIText.cpp:178:2: remark: _ZNK8CGUIText16ComputeLineRangeERKSt5arrayISt6vectorI18SGenerateTextImageSaIS2_EELm2EEfffRfS8_ inlined into

  • ComputeLineSize is not inlined
  • GetLineOffset is inlined:

../../../source/gui/CGUIText.cpp:192:19: remark: _ZNK8CGUIText13GetLineOffsetE6EAlignffRK5CSize/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.1.0/../../../../include/c++/9.1.0/bits/stl_tree.h:2517 inlined :22: remark: _ZNSt23_Rb_tree_const_iteratorISt4pairIK5CStr8PK10CGUISpriteEEC2ERKSt17_Rb_tree_iteratorIS6_E inlined into

  • SetupSpriteCalls is not inlined
  • ProcessLine is not inlined
  • AssembleCalls is not inlined

Notice those are not exactly the functions that are const, nor are those the functions without mutable arguments.

source/gui/CGUI.cpp
498 ↗(On Diff #9289)

code including TODO comments from rP290

550 ↗(On Diff #9289)

WordWrapping proxy removed to pass fewer (not less) arguments around

560 ↗(On Diff #9289)

Care, this done was removed.

625 ↗(On Diff #9289)

deleted ugly pointless local statics, using 2 floats instead of float[2]

837 ↗(On Diff #9289)

Rename to Draw since the Text is redundant with CGUIText otherwise

854 ↗(On Diff #9289)

This looks like a defect to construct this instance every freakin call to Draw, introduced in rP10985.

868 ↗(On Diff #9289)

28 floor( or floorf( instances, 0 std::floor instances, so going for the "math.h" intead of <cmath> variant.

source/gui/CGUIText.h
198 ↗(On Diff #9289)

Passing Feedback.m_Images instead of the Feedback of type CGUIString::SFeedback is a trick to make it compile, because one can't forward declare nested classes and because the two headers can't include / depend on each other.

elexis updated this revision to Diff 9290.Aug 11 2019, 6:12 PM

Rename Draw, floor, static_cast, whitespace, clang warning, restore a TODO

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

Linter detected issues:
Executing section Source...

source/gui/IGUITextOwner.h
|  23| »   Interface·class·that·enhance·the·IGUIObject·with
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIList{' is invalid C code. Use --std or --language to configure the language.

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/GUI.h
|  23| #include·<string>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIList{' is invalid C code. Use --std or --language to configure the language.

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

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

source/gui/CGUIText.h
|  48| class·CGUIText
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIText{' 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/354/display/redirect

I got another

../../../source/gui/CButton.cpp:72:24: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
        m_GeneratedTexts[0] = std::move(CGUIText(m_pGUI, *caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this));
                              ^
../../../source/gui/CButton.cpp:72:24: note: remove std::move call here
        m_GeneratedTexts[0] = std::move(CGUIText(m_pGUI, *caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this));
                              ^~~~~~~~~~                                                                                  ~
../../../source/gui/CCheckBox.cpp:85:24: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
        m_GeneratedTexts[0] = std::move(CGUIText(m_pGUI, *caption, font, m_CachedActualSize.GetWidth() - square_side, 0.f, this));
                              ^
../../../source/gui/CCheckBox.cpp:85:24: note: remove std::move call here
        m_GeneratedTexts[0] = std::move(CGUIText(m_pGUI, *caption, font, m_CachedActualSize.GetWidth() - square_side, 0.f, this));
                              ^~~~~~~~~~                                                                                        ~

I was wondering already why I got _no_ compiler warning by gcc about that which deos warn about return std::move(T)

Didn't I get build errors on GCC complaining that this would use the disabled copy assignment?

It was a mystery why clang wasn't inlining.
For example for ComputeLineSize:

  • Only called in one place
  • Same translation unit
  • Doesn't matter if its a local cpp function or part of the class
  • Tried changing the CSize to 2x float
  • function has only 1 loop and 9 statements, no return value
  • Spammed inline keyword to every function

Nothing made a difference.

Then I tried compiling with gcc and having it show warnings when it does not inline:

CC="gcc -Winline" CXX="gcc -Winline" make -j4

I've added the keyword to the GUIText::Draw function as well to confirm that the warnings work (as this function cannot be inlined since it is in a different translation unit).
The result is that there were no warnings except the one about the Draw function in this file, meaning gcc does inline.

In file included from ../../../source/gui/CGUIString.h:22,
                 from ../../../source/gui/CGUIList.h:21,
                 from ../../../source/gui/GUI.h:28,
                 from ../../../source/pch/gui/precompiled.h:28:
../../../source/gui/CGUIText.h:170:14: warning: inline function ‘void CGUIText::Draw(CGUI*, const CGUIColor&, const CPos&, float, const CRect&) const’ used but never defined
  170 |  inline void Draw(CGUI* pGUI, const CGUIColor& DefaultColor, const CPos& pos, const float z, const CRect& clipping) const;
      |              ^~~~
../../../source/gui/CGUIText.h: In member function ‘virtual void IGUITextOwner::DrawText(size_t, const CGUIColor&, const CPos&, float, const CRect&)’:
../../../source/gui/CGUIText.h:170:14: warning: inlining failed in call to ‘void CGUIText::Draw(CGUI*, const CGUIColor&, const CPos&, float, const CRect&) const’: function body not available [-Winline]
../../../source/gui/IGUITextOwner.cpp:101:33: note: called from here
  101 |  m_GeneratedTexts.at(index).Draw(m_pGUI, color, pos, z, clipping);
      |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So it seems with default optimization levels, gcc is smart enough, but clang only smartness 2/5 (or just different random tresholds that don't make much of a difference.)

elexis edited the test plan for this revision. (Show Details)Aug 11 2019, 9:18 PM

Same result with -O3:

CC="clang -Rpass=inline -O3" CXX="clang++ -Rpass=inline -O3" make -j4

elexis updated this revision to Diff 9296.Aug 11 2019, 10:28 PM

Remove the std::move preventing copy elision.

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

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

I did a benchmark test by calling the proposed constructor 5000 times in a row, and the previous function 5000 times, in the same program.

When I did with Clang that only inlines 2 of the 5 functions:

STRING: Enable Feedback
prop took 13045277
prev took 10193902
STRING: Terms
prop took 5964622
prev took 5955610
STRING: 
prop took 137344
prev took 652167
STRING: Learn to Play
prop took 13918847
prev took 13860921
STRING: Single Player
prop took 10350261
prev took 10320037
STRING: Multiplayer
prop took 6349635
prev took 6354328
STRING: Settings
prop took 6197764
prev took 6244866
STRING: Scenario Editor
prop took 10581746
prev took 10562483
STRING: Exit
prop took 6039952
prev took 6064610
STRING: Website
prop took 6341808
prev took 6385139
STRING: Chat
prop took 6083446
prev took 6178647
STRING: Report a Bug
prop took 13589212
prev took 13676114
STRING: Translate the Game
prop took 14843883
prev took 14735341
STRING: Donate
prop took 6292475
prev took 6244785
STRING: Credits
prop took 6315602
prev took 6301848
STRING: Manual
prop took 6326105
prev took 6353327
STRING: Tutorial
prop took 6424886
prev took 6483412
STRING: Structure Tree
prop took 11003499
prev took 10973671
STRING: History
prop took 6384237
prev took 6401929
STRING: Manual
prop took 6327015
prev took 6272400
STRING: Tutorial
prop took 6299790
prev took 6295394
STRING: Structure Tree
prop took 10578165
prev took 10634730
STRING: History
prop took 6203418
prev took 6244512
STRING: Manual
prop took 6337480
prev took 6275598
STRING: Tutorial
prop took 6460232
prev took 6489275
STRING: Structure Tree
prop took 10878545
prev took 10918467
STRING: History
prop took 6408610
prev took 6435106
STRING: Manual
prop took 6224131
prev took 6220124
STRING: Tutorial
prop took 6363911
prev took 6419837
STRING: Structure Tree
prop took 10683471
prev took 10723261
STRING: History
prop took 6284849
prev took 6353074
STRING: 
prop took 136296
prev took 669016
STRING: 
prop took 132270
prev took 662765
STRING: 
prop took 131553
prev took 688868
STRING: 
prop took 131537
prev took 678945
STRING: 
prop took 133031
prev took 652371
STRING: 
prop took 134343
prev took 641704
STRING: 
prop took 131734
prev took 633359
STRING: 
prop took 131669
prev took 634703
STRING: 
prop took 136366
prev took 646433
STRING: 
prop took 133015
prev took 664062
STRING: 
prop took 136051
prev took 668671
STRING: 
prop took 134367
prev took 674086
STRING: 
prop took 136144
prev took 662181
STRING: 
prop took 139692
prev took 675460
STRING: 
prop took 136239
prev took 667100
STRING: 
prop took 129900
prev took 674012
STRING: 
prop took 135828
prev took 682841
STRING: 
prop took 128425
prev took 675437
STRING: 
prop took 136033
prev took 673074
STRING: 
prop took 134676
prev took 676005
STRING: 
prop took 131624
prev took 673005
STRING: 
prop took 131623
prev took 667234
STRING: 
prop took 144853
prev took 668672
STRING: 
prop took 131548
prev took 684232
STRING: 
prop took 131655
prev took 640629
STRING: 
prop took 134582
prev took 662634
STRING: 
prop took 132592
prev took 643616
STRING: 
prop took 135916
prev took 633257
STRING: 
prop took 135960
prev took 676891
STRING: 
prop took 129903
prev took 668121
STRING: 
prop took 131601
prev took 668641
STRING: 
prop took 131777
prev took 654108
STRING: 
prop took 128565
prev took 664193
STRING: 
prop took 135993
prev took 658362
STRING: 
prop took 134482
prev took 661240
STRING: 
prop took 144580
prev took 681805
STRING: 
prop took 131519
prev took 649407
STRING: 
prop took 135848
prev took 639172
STRING: 
prop took 135932
prev took 653747
STRING: 
prop took 149497
prev took 662766
STRING: 
prop took 128594
prev took 645053
STRING: 
prop took 135908
prev took 636104
STRING: 
prop took 128546
prev took 645045
STRING: 
prop took 134645
prev took 689366
STRING: 
prop took 129968
prev took 675556
STRING: 
prop took 136014
prev took 651399
STRING: 
prop took 128720
prev took 673082
STRING: 
prop took 135860
prev took 656823
STRING: 
prop took 131449
prev took 677632
STRING: 
prop took 135809
prev took 645994
STRING: 
prop took 129962
prev took 664081
STRING: 
prop took 135880
prev took 702653
STRING: 
prop took 164006
prev took 680555
STRING: 
prop took 131476
prev took 664239
STRING: 
prop took 131571
prev took 659745
STRING: 
prop took 133246
prev took 679503
STRING: 
prop took 134513
prev took 642098
STRING: 
prop took 135915
prev took 643561
STRING: 
prop took 141739
prev took 693255
STRING: 
prop took 131500
prev took 653760
STRING: 
prop took 134640
prev took 661170
STRING: 
prop took 136039
prev took 666457
STRING: 
prop took 135775
prev took 635687
STRING: 
prop took 131382
prev took 644452
STRING: 
prop took 135772
prev took 643002
STRING: 
prop took 131620
prev took 644271
STRING: 
prop took 153618
prev took 677579
STRING: 
prop took 133052
prev took 645156
STRING: 
prop took 135903
prev took 634694
STRING: 
prop took 135991
prev took 646543
STRING: 
prop took 134551
prev took 659718
STRING: 
prop took 131557
prev took 639082
STRING: 
prop took 131447
prev took 661777
STRING: 
prop took 130029
prev took 645048
STRING: 
prop took 134372
prev took 653444
STRING: 
prop took 131372
prev took 650462
STRING: 
prop took 130196
prev took 649360
STRING: 
prop took 131536
prev took 655345
STRING: 
prop took 134577
prev took 659812
STRING: 
prop took 130154
prev took 647092
STRING: 
prop took 136009
prev took 645106
STRING: 
prop took 128632
prev took 637676
STRING: 
prop took 135949
prev took 691152
STRING: 
prop took 131597
prev took 652393
STRING: 
prop took 135927
prev took 658291
STRING: 
prop took 135817
prev took 693234
STRING: 
prop took 129921
prev took 693521
STRING: 
prop took 134576
prev took 662735
STRING: 
prop took 131556
prev took 659688
STRING: 
prop took 131570
prev took 649034
STRING: 
prop took 135870
prev took 676015
STRING: 
prop took 131469
prev took 638989
STRING: 
prop took 132907
prev took 637698
STRING: 
prop took 128646
prev took 676004
STRING: 
prop took 131638
prev took 670117
STRING: 
prop took 131675
prev took 661433
STRING: 
prop took 130071
prev took 643973
STRING: 
prop took 131402
prev took 650478
STRING: 
prop took 135926
prev took 703974
STRING: 
prop took 135754
prev took 649005
STRING: 
prop took 134493
prev took 678917
STRING: 
prop took 134342
prev took 703499
STRING: 
prop took 131493
prev took 669533
STRING: 
prop took 131518
prev took 668759

I tested with this diff:

Index: source/gui/CButton.cpp
===================================================================
--- source/gui/CButton.cpp	(revision 22644)
+++ source/gui/CButton.cpp	(working copy)
@@ -45,12 +45,11 @@ CButton::CButton(CGUI* pGUI)
 	AddSetting<CGUIColor>("textcolor_pressed");
 	AddSetting<CGUIColor>("textcolor_disabled");
 	AddSetting<CStrW>("tooltip");
 	AddSetting<CStr>("tooltip_style");
 
-	// Add text
-	AddText(new SGUIText());
+	AddText();
 }
 
 CButton::~CButton()
 {
 }
@@ -68,13 +67,28 @@ void CButton::SetupText()
 	CGUIString* caption = nullptr;
 	GUI<CGUIString>::GetSettingPointer(this, "caption", caption);
 
 	float buffer_zone = 0.f;
 	GUI<float>::GetSetting(this, "buffer_zone", buffer_zone);
-	*m_GeneratedTexts[0] = GetGUI()->GenerateText(*caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this);
 
-	CalculateTextPosition(m_CachedActualSize, m_TextPos, *m_GeneratedTexts[0]);
+	{
+		auto t1 = std::chrono::high_resolution_clock::now();
+		for (int i = 0; i < 5000; ++i)
+			m_GeneratedTexts[0] = CGUIText(m_pGUI, *caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this);
+		auto t2 = std::chrono::high_resolution_clock::now();
+		debug_printf("prop took %d\n", (t2 - t1).count());
+	}
+
+	{
+		auto t1 = std::chrono::high_resolution_clock::now();
+		for (int i = 0; i < 5000; ++i)
+			m_GeneratedTexts[0] = m_pGUI->GenerateText(*caption, font, m_CachedActualSize.GetWidth(), buffer_zone, this);
+		auto t2 = std::chrono::high_resolution_clock::now();
+		debug_printf("prev took %d\n", (t2 - t1).count());
+	}
+
+	CalculateTextPosition(m_CachedActualSize, m_TextPos, m_GeneratedTexts[0]);
 }
 
 void CButton::HandleMessage(SGUIMessage& Message)
 {
 	// Important

So we see on Clang that only inlines 2 of 5 functions, it's 0.5% to 1% faster than before for non-empty strings,
and for the empty string its 5x faster.

That must be the cost of copying an empty object.
500k ticks.

Tested the same with gcc, that's a similar result, sometimes faster, sometimes slower, whereas on clang it was always faster:

STRING: Enable Feedback
prop took 13994146
prev took 10286451
STRING: Terms
prop took 5608717
prev took 5637500
STRING: 
prop took 147675
prev took 649044
STRING: Learn to Play
prop took 13204377
prev took 12932286
STRING: Single Player
prop took 9759912
prev took 10115043
STRING: Multiplayer
prop took 6225207
prev took 6220658
STRING: Settings
prop took 5778485
prev took 5934261
STRING: Scenario Editor
prop took 10034152
prev took 9991759
STRING: Exit
prop took 5648364
prev took 5815125
STRING: Website
prop took 5815562
prev took 5757285
STRING: Chat
prop took 5697139
prev took 5707167
STRING: Report a Bug
prop took 12905320
prev took 12811983
STRING: Translate the Game
prop took 14044238
prev took 14117801
STRING: Donate
prop took 5844190
prev took 5817753
STRING: Credits
prop took 5792525
prev took 5839650
STRING: Enable Feedback
prop took 10013300
prev took 10365117
STRING: Terms
prop took 5708653
prev took 5812871
STRING: 
prop took 147819
prev took 634892
STRING: Learn to Play
prop took 14123223
prev took 13376768
STRING: Single Player
prop took 9916619
prev took 10077662
STRING: Multiplayer
prop took 6162276
prev took 6126512
STRING: Settings
prop took 5841792
prev took 6016395
STRING: Scenario Editor
prop took 10156002
prev took 10177304
STRING: Exit
prop took 5674363
prev took 5815699
STRING: Website
prop took 5712013
prev took 5757011
STRING: Chat
prop took 5654077
prev took 5708243
STRING: Report a Bug
prop took 12834216
prev took 12719330
STRING: Translate the Game
prop took 14154056
prev took 14168726
STRING: Donate
prop took 5748066
prev took 5778934
STRING: Credits
prop took 5755106
prev took 5822836
STRING: Manual
prop took 6089204
prev took 6076452
STRING: Tutorial
prop took 6213008
prev took 6142854
STRING: Structure Tree
prop took 10916779
prev took 10586563
STRING: History
prop took 6288267
prev took 6041560
STRING: Manual
prop took 6129823
prev took 5993514
STRING: Tutorial
prop took 6115702
prev took 6332570
STRING: Structure Tree
prop took 10508629
prev took 10618142
STRING: History
prop took 6061026
prev took 6224357
STRING: Manual
prop took 6328042
prev took 6023033
STRING: Tutorial
prop took 6137679
prev took 6384850
STRING: Structure Tree
prop took 10423959
prev took 10617938
STRING: History
prop took 5981608
prev took 5991860
STRING: 
prop took 143215
prev took 632716
STRING: 
prop took 151291
prev took 621792
STRING: 
prop took 143740
prev took 623483
STRING: 
prop took 145903
prev took 651151
STRING: 
prop took 143702
prev took 620425
STRING: 
prop took 154628
prev took 628521
STRING: 
prop took 142385
prev took 632291
STRING: 
prop took 149439
prev took 647425
STRING: 
prop took 144924
prev took 623002
STRING: 
prop took 147877
prev took 640783
STRING: 
prop took 156490
prev took 633503
STRING: 
prop took 144695
prev took 623929
STRING: 
prop took 137308
prev took 637104
STRING: 
prop took 149098
prev took 628235
STRING: 
prop took 137652
prev took 767719
STRING: 
prop took 144675
prev took 649059
STRING: 
prop took 144732
prev took 635742
STRING: 
prop took 140283
prev took 625351
STRING: 
prop took 145206
prev took 629440
STRING: 
prop took 145183
prev took 621916
STRING: 
prop took 158356
prev took 622032
STRING: 
prop took 149479
prev took 629219
STRING: 
prop took 148052
prev took 683146
STRING: 
prop took 145565
prev took 626178
STRING: 
prop took 142286
prev took 623300
STRING: 
prop took 144997
prev took 632036
STRING: 
prop took 144944
prev took 618785
STRING: 
prop took 145425
prev took 619095
STRING: 
prop took 149467
prev took 632712
STRING: 
prop took 149078
prev took 625477
STRING: 
prop took 142326
prev took 629289
STRING: 
prop took 145136
prev took 623401
STRING: 
prop took 137738
prev took 646775
STRING: 
prop took 149456
prev took 627532
STRING: 
prop took 143647
prev took 636512
STRING: 
prop took 152484
prev took 643927
STRING: 
prop took 155427
prev took 666202
STRING: 
prop took 147870
prev took 645097
STRING: 
prop took 149500
prev took 621896
STRING: 
prop took 145178
prev took 623322
STRING: 
prop took 137746
prev took 621988
STRING: 
prop took 149397
prev took 649752
STRING: 
prop took 148568
prev took 683321
STRING: 
prop took 145151
prev took 663392
STRING: 
prop took 147601
prev took 629855
STRING: 
prop took 145386
prev took 623256
STRING: 
prop took 137857
prev took 623305
STRING: 
prop took 159808
prev took 618850
STRING: 
prop took 165075
prev took 624138
STRING: 
prop took 149116
prev took 622397
STRING: 
prop took 143460
prev took 634361
STRING: 
prop took 145137
prev took 618855
STRING: 
prop took 143483
prev took 627750
STRING: 
prop took 147932
prev took 644661
STRING: 
prop took 148017
prev took 620504
STRING: 
prop took 148567
prev took 660169
STRING: 
prop took 143691
prev took 658680
STRING: 
prop took 147810
prev took 633412
STRING: 
prop took 142922
prev took 622969
STRING: 
prop took 145142
prev took 651968
STRING: 
prop took 143871
prev took 646461
STRING: 
prop took 145035
prev took 655961
STRING: 
prop took 149150
prev took 625465
STRING: 
prop took 146171
prev took 655301
STRING: 
prop took 159137
prev took 624267
STRING: 
prop took 147879
prev took 618660
STRING: 
prop took 149570
prev took 627884
STRING: 
prop took 144943
prev took 629054
STRING: 
prop took 149375
prev took 655275
STRING: 
prop took 149291
prev took 709699
STRING: 
prop took 157603
prev took 630537
STRING: 
prop took 148190
prev took 649351
STRING: 
prop took 148079
prev took 649497
STRING: 
prop took 145075
prev took 670500
STRING: 
prop took 145668
prev took 643455
STRING: 
prop took 146176
prev took 635683
STRING: 
prop took 171746
prev took 633467
STRING: 
prop took 140842
prev took 618871
STRING: 
prop took 145065
prev took 652964
STRING: 
prop took 143947
prev took 635021
STRING: 
prop took 144979
prev took 624545
STRING: 
prop took 137564
prev took 628937
STRING: 
prop took 145188
prev took 618931
STRING: 
prop took 140964
prev took 626023
STRING: 
prop took 149325
prev took 647647
STRING: 
prop took 149150
prev took 662233
STRING: 
prop took 147592
prev took 631238
STRING: 
prop took 143614
prev took 657395
STRING: 
prop took 148061
prev took 633973
STRING: 
prop took 148163
prev took 617802
STRING: 
prop took 146517
prev took 634412
STRING: 
prop took 162591
prev took 641654
STRING: 
prop took 174470
prev took 633421
STRING: 
prop took 149711
prev took 633480
STRING: 
prop took 146008
prev took 631909
STRING: 
prop took 140854
prev took 623341
STRING: 
prop took 186962
prev took 631136
STRING: 
prop took 143193
prev took 619503
STRING: 
prop took 150734
prev took 646947
STRING: 
prop took 147708
prev took 622594
STRING: 
prop took 144467
prev took 623057
STRING: 
prop took 143286
prev took 623861
STRING: 
prop took 143371
prev took 622372
STRING: 
prop took 148154
prev took 619088
STRING: 
prop took 144646
prev took 618049
STRING: 
prop took 145867
prev took 630457
STRING: 
prop took 140589
prev took 633225
STRING: 
prop took 152530
prev took 633730
STRING: 
prop took 143672
prev took 642412
STRING: 
prop took 148174
prev took 626409
STRING: 
prop took 148048
prev took 624646
STRING: 
prop took 147905
prev took 627430
STRING: 
prop took 143595
prev took 632312
STRING: 
prop took 158670
prev took 651297
STRING: 
prop took 146076
prev took 626315
STRING: 
prop took 149767
prev took 640796
STRING: 
prop took 144981
prev took 627517
STRING: 
prop took 148608
prev took 677217
STRING: 
prop took 145352
prev took 629250
STRING: 
prop took 139463
prev took 620380
STRING: 
prop took 144868
prev took 627555
STRING: 
prop took 140608
prev took 845832
STRING: 
prop took 145027
prev took 648154
STRING: 
prop took 147897
prev took 648130
STRING: 
prop took 140699
prev took 633228
STRING: 
prop took 144943
prev took 628873
STRING: 
prop took 142136
prev took 665861
STRING: 
prop took 145214
prev took 621836
STRING: 
prop took 145158
prev took 633614
STRING: 
prop took 143503
prev took 669629
STRING: 
prop took 145034
prev took 648224
STRING: 
prop took 148024
prev took 618915
STRING: 
prop took 149527
prev took 621912
STRING: 
prop took 145149
prev took 683615
STRING: 
prop took 149071
prev took 626804
STRING: 
prop took 148067
prev took 650201
STRING: 
prop took 141743
prev took 619661
STRING: 
prop took 144701
prev took 644796
STRING: 
prop took 143156
prev took 616461
STRING: History
prop took 5753710
prev took 5786340
STRING: Close
prop took 5750724
prev took 5742697
vladislavbelov added inline comments.
source/gui/CGUIString.h
25 ↗(On Diff #9296)

Why <list> is included, but <array> not? The same below.

source/gui/CGUIText.cpp
28 ↗(On Diff #9296)

Maybe <cmath>?

source/gui/CGUIText.h
26 ↗(On Diff #9296)

Wrong order.

142 ↗(On Diff #9296)

We usually put NONCOPYABLE in the private (or public for struct) section right after the scope begin.

227 ↗(On Diff #9296)

What does i mean here?

elexis added inline comments.Aug 12 2019, 1:51 AM
source/gui/CGUIString.h
25 ↗(On Diff #9296)

(01:25:37 AM) elexis: I need to update the diff
(01:25:49 AM) elexis: as the uploaded one is broken and also some header thing

:P

source/gui/CGUIText.cpp
28 ↗(On Diff #9296)

There are:
https://en.cppreference.com/w/c/numeric/math/floor
https://en.cppreference.com/w/cpp/numeric/math/floor

I've chosen the c variant, since there are 28 occurrences thereof and 0 of the std variant (https://code.wildfiregames.com/D2168#inline-42473)

source/gui/CGUIText.h
142 ↗(On Diff #9296)

I put it here in order to group it with MOVABLE, and I put MOVABLE here because it defines a constructor, and to group the constructor with the other methods and members

227 ↗(On Diff #9296)

The same meaning as in all of the functions, the i'th word

string.m_Words[i]

It's only assigned in 2 places of the code, one is the iterator, one changing it by -1 to process the same word again on the next line

elexis updated this revision to Diff 9297.Aug 12 2019, 1:54 AM

Added includes, missing const keyword

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

Linter detected issues:
Executing section Source...

source/gui/GUI.h
|  23| #include·<string>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIList{' is invalid C code. Use --std or --language to configure the language.

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

source/gui/IGUITextOwner.h
|  23| »   Interface·class·that·enhance·the·IGUIObject·with
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIList{' is invalid C code. Use --std or --language to configure the language.

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

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/CGUIText.h
|  51| class·CGUIText
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIText{' 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/360/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Aug 17 2019, 3:32 AM
This revision was automatically updated to reflect the committed changes.