Page MenuHomeWildfire Games

Fix build without precompiled headers following Atlas UTF8 fix rP22335 and ThreadUtil fix rP22344
ClosedPublic

Authored by elexis on Jul 5 2019, 6:47 PM.

Details

Summary

When compiling without precompiled headers one can notice a compiler warning following rP22335:

In file included from ../../../source/tools/atlas/GameInterface/ActorViewer.cpp:20:
../../../source/tools/atlas/GameInterface/ActorViewer.h:38:39: error: ‘CStr’ does not name a type
   38 |  void SetActor(const CStrW& id, const CStr& animation, player_id_t playerID);

../../../source/tools/atlas/GameInterface/ActorViewer.cpp: At global scope:
../../../source/tools/atlas/GameInterface/ActorViewer.cpp:331:6: error: no declaration matches ‘void ActorViewer::SetActor(const CStrW&, const CStr8&, player_id_t)’
  331 | void ActorViewer::SetActor(const CStrW& name, const CStr& animation, player_id_t playerID)

and following rP22344 one gets:

In file included from ../../../source/lib/precompiled.h:71,
                 from ../../../source/pch/engine/precompiled.h:18,
                 from ../../../source/ps/CStrIntern.cpp:18:
../../../source/ps/CStrIntern.cpp: In function ‘CStrInternInternals* GetString(const char*, size_t)’:
../../../source/ps/CStrIntern.cpp:100:9: error: ‘ThreadUtil’ has not been declared
  100 |  ENSURE(ThreadUtil::IsMainThread());
      |         ^~~~~~~~~~
../../../source/lib/debug.h:291:8: note: in definition of macro ‘ENSURE’
  291 |   if(!(expr))\
      |        ^~~~
make[1]: *** [engine.make:276: obj/engine_Release/CStrIntern.o] Error 1
make: *** [Makefile:109: engine] Error 2
make: *** Waiting for unfinished jobs....
Test Plan

Consult https://trac.wildfiregames.com/wiki/BuildInstructions for how to build without pch.

Diff Detail

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

Event Timeline

elexis created this revision.Jul 5 2019, 6:47 PM
wraitii accepted this revision.Jul 5 2019, 6:50 PM
wraitii added a subscriber: wraitii.

Correct fix, and animation is indeed a CStr8. The function implementation has "name" for the first argument which seems better to changing is OK.

This revision is now accepted and ready to land.Jul 5 2019, 6:50 PM
Vulcan added a comment.Jul 5 2019, 6:54 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1904/display/redirect

Why you didn't change implementation too?

elexis retitled this revision from Fix build without precompiled headers following Atlas UTF8 fix rP22335 to Fix build without precompiled headers following Atlas UTF8 fix rP22335 and ThreadUtil fix rP22344.Jul 7 2019, 8:33 PM
elexis edited the summary of this revision. (Show Details)
elexis edited the summary of this revision. (Show Details)Jul 7 2019, 8:50 PM
elexis updated this revision to Diff 8768.Jul 7 2019, 8:56 PM

Include fix for rP22344, use include instead of switching to CStr8 in header and implementation for global consistency.

Vulcan added a comment.Jul 7 2019, 8:58 PM

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

Linter detected issues:
Executing section Source...

source/ps/CStrIntern.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1937/display/redirect

elexis added a comment.Jul 7 2019, 8:59 PM

Notice class CStr; doesn't compile, so the choice is either to use a forward declaration for CStr8 or to include CStr.h.
Using almost always CStr in the implementation but only rarely CStr8 seems wrong (Coding Convention speak about consistency).

maths/Fixed.h:class CStr8;
graphics/Terrain.h:class CStr8;
graphics/SkeletonAnimManager.h:class CStr8;
graphics/Color.h:class CStr8;
graphics/ColladaManager.h:class CStr8;
graphics/UnitManager.h:class CStr8;
ps/Profile.h:class CStr8;
ps/KeyName.h:class CStr8;
ps/CStr.h:class CStr8;

Hence I'm proposing including the header now.

grep -R 'include "ps/CStr.h"' | wc -l
88
elexis updated this revision to Diff 8771.Jul 7 2019, 11:34 PM

Update year, sorting order

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1939/display/redirect

vladislavbelov accepted this revision.Jul 7 2019, 11:45 PM

Reviewed on IRC (from few days).

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jul 7 2019, 11:54 PM