Skip to content

Refactor/string ids#985

Merged
AaronVanGeffen merged 4 commits intoOpenLoco:masterfrom
rdrdrdrd95:REFACTOR/StringIds
Jul 1, 2021
Merged

Refactor/string ids#985
AaronVanGeffen merged 4 commits intoOpenLoco:masterfrom
rdrdrdrd95:REFACTOR/StringIds

Conversation

@rdrdrdrd95
Copy link
Copy Markdown
Contributor

Open issue:

Reduce inclusion of StringIds.h in header files #741

@duncanspumpkin
Copy link
Copy Markdown
Contributor

Unfortunately due to constexprs being like templates and inline in that they must be immediately defined you can't use extern and must have the full definition in the header. It makes this pr just the deletion of one or two #includes.

@rdrdrdrd95
Copy link
Copy Markdown
Contributor Author

I thought Microsoft fixed that:

https://docs.microsoft.com/en-us/cpp/build/reference/zc-externconstexpr?view=msvc-160

Assuming this does not work, do these functions actually need to be constexpr? Is the benefit of marginal run time performance vs compile time worth it here?

@duncanspumpkin
Copy link
Copy Markdown
Contributor

The widget one definitely needs to stay constexpr the vehicle one I'm not sure. Even if VS is happy with it afaik it's not standards compliant so it will probably not work on gcc and clang.

rdrdrdrd95 and others added 2 commits July 1, 2021 08:38
Converted getVehicleType to use const input

Builds with #include <path>/StringIds.h removed from all headers

removed comment

cleaning up include order
@duncanspumpkin
Copy link
Copy Markdown
Contributor

@rdrdrdrd95 I've taken this PR and sort of reverted all of the constexpr extern changes. Sorry we should have really thought through the issue before putting it up as a good first issue.

@AaronVanGeffen AaronVanGeffen merged commit 17f1d1f into OpenLoco:master Jul 1, 2021
@rdrdrdrd95
Copy link
Copy Markdown
Contributor Author

Thanks for following up on that, I have been swamped on-boarding our new college hires for the past few weeks.

tomasharkema added a commit to tomasharkema/OpenLoco that referenced this pull request Jul 13, 2021
…ature/mac-docker

* 'master' of https://github.com/OpenLoco/OpenLoco: (536 commits)
  Mention code style in readme (OpenLoco#1047)
  Remove redundant rotate function (OpenLoco#1046)
  Link up implemented function (OpenLoco#1043)
  Fix use of constants for coord limits (OpenLoco#1037)
  validCoords: remove template parameter and name type (OpenLoco#1033)
  Add coordinate validation to tile loops with offsets (OpenLoco#1031)
  Implement ChangeCompanyColour game command (OpenLoco#1029)
  Implement colour picker dropdowns (OpenLoco#1028)
  Implement StationManager::generateNewStationName (OpenLoco#1020)
  Station rename command (OpenLoco#984)
  Fix access to embedded object name (OpenLoco#1022)
  Reduce inclusion of StringIds.h in header files (OpenLoco#985)
  Implement the station name background paint (OpenLoco#1023)
  Force alignment on TileManager::createAnimation hook
  Merge duplicated flags and use accessors (OpenLoco#1025)
  Remove use of global stringformatbuffer where not required (OpenLoco#1024)
  Refactor calls to tryCreateInitialMovementSound (OpenLoco#1018)
  Turn widget draw functions into Widget struct member functions (OpenLoco#1012)
  Allow filtering the vehicle list by station or cargo (OpenLoco#997)
  Restore game command table alignment
  ...
@AaronVanGeffen AaronVanGeffen added this to the v21.07 milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants