Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move always.h from headers to cpp files #1121

Closed
wants to merge 1 commit into from

Conversation

xezon
Copy link
Contributor

@xezon xezon commented Feb 17, 2024

This change aims to further reduce compile times.

On my machine with SSD, this reduces compile time in Debug configuration by 1 second - from 42 seconds to 41 seconds.

Disadvantage of this change

Most headers will no longer compile on their own when included in a cpp file. Every cpp file now needs to have always.h included at the very top. Our clang-format already sorts it that way.

Advantage of this change

This principle causes less file includes and a bit faster compilation.

Also, it allows the project to use precompiled headers moving forward. For next steps, we can move always.h into a stdafx.h, and then bundle it with some other hot files such as xfer.h and asciistring.h, and include stdafx.h instead of always.h in all cpp files.

https://en.wikipedia.org/wiki/Precompiled_header

precomp

@xezon xezon force-pushed the fix-always-header branch 3 times, most recently from d98a03f to 2b23e6d Compare February 24, 2024 11:30
@xezon xezon force-pushed the fix-always-header branch from 2b23e6d to 4c6df87 Compare February 24, 2024 11:37
@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 2.54%. Comparing base (b013231) to head (4c6df87).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1121   +/-   ##
========================================
  Coverage     2.54%    2.54%           
========================================
  Files          949      949           
  Lines       110299   110299           
  Branches     18881    18881           
========================================
  Hits          2802     2802           
  Misses      107094   107094           
  Partials       403      403           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xezon xezon requested a review from OmniBlade February 24, 2024 14:48
Copy link
Contributor

@OmniBlade OmniBlade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so my concerns are 1. I think headers compiling on their own is a useful feature and they should include anything they require. 2. The precompiled header benefits only apply to Windows while we aspire to have the code base compile cross platform. In a similar project https://github.com/OmniBlade/LasMarionetas/tree/master/src/compat I've split what always.h encapsulates into multiple headers and just avoided the macros for platforms in favour of the compiler provided ones.

@xezon
Copy link
Contributor Author

xezon commented Mar 4, 2024

I don't feel strongly about this change. We can close it.

As for multi platform, Windows by far is the most important platform because this is what 99% of the actual players use, so any development improvements for Windows platform are worthwhile improvements.

@xezon xezon closed this Mar 6, 2024
@xezon xezon deleted the fix-always-header branch March 6, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants