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

Restructure + Build-Time Reduction #7

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

weigert
Copy link

@weigert weigert commented Jul 6, 2023

This PR is intended to reduce the build time of the graphics portion of dwarf fortress, by more cleanly separating translation units and dependencies. This has the added benefit of improving legibility for other potential contributions.

Note that no actual implementation code has been altered - just moving declarations and definitions around.

This branch should be merged after the glue branch is merged. I am opening this PR now, to check acceptance for a restructure of this type. My hope is that with this, the actual graphics portion can be tidied up more easily since many inter-code dependencies have been wrangled.

Since many files were moved and then altered, I recommend just looking at the source code of the merging branch and decide whether this seems reasonable. Then, if everything outside of this repository compiles, that would be awesome.

Key Changes:

  • Overall Build-Time Reduction: 55s -> 49s (MacOS, Note: Not minimal yet. Graphics portion still needs splitting)
  • Uniform include guards across the repository
  • Reduced include complexity by unwinding dependencies
  • Cleaner separation of code into directories for logical separation (e.g. utils, files, etc.)
  • Uniform line indentation: 2 spaces (Note: Not linted, no uniform bracketing style since it was all over the place)

weigert and others added 23 commits April 10, 2023 11:17
…extra linkage and g++11 which has library support for semaphore and syncstream. removed self-credit in Makefile
…extra linkage and g++11 which has library support for semaphore and syncstream. removed self-credit in Makefile
…r to belong structure wise. With this, some linting, include guarding and dependency reduction for compile-time improvement can be done. Compile time is roughly 55s currently
…unctions and logging. compile times are now reduced to 49s
@shevernitskiy
Copy link

shevernitskiy commented Jul 13, 2023

@weigert, since you are introduce uniform indentation, maybe it will be reasonable to introduce .clang-format as well to more consistent code style for existing code base and potential further contributions?

ps: found a bit intend dance here https://github.com/weigert/Dwarf-Fortress--libgraphics--/blob/weigert_restructure/g_src/graphics/enabler.cpp#L892 :)

…e.g. for separating definition blocks. The code is now uniform style and very legible.
@weigert
Copy link
Author

weigert commented Jul 15, 2023

@shevernitskiy added .clang-format and reformatted the code accordingly. Now the style is necessarily uniform and the code is suddenly very legible.

In case these changes are too dramatic for @Putnam3145, leading to possible merge conflicts in the future, they can also just steal the .clang-format file and use that directly on a clean head.

Still the benefits from the header unwinding are still there, as well as the code structure resolution.

In a subsequent move, all of this could be integrated with GitHub actions so that the engine code for dwarf fortress becomes super maintainable.

@shevernitskiy
Copy link

So much better to read, for me style looks good. Just one thing - column limit to 100 or 120 (i suppose we all have at least 16:9 monitors:)).
I think, @Putnam3145 should adjust rules according personal taste as a main maintainer.

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.

2 participants