CMake: Grab bag of tiny refactors and improvements#1683
Closed
enetheru wants to merge 9 commits into
Closed
Conversation
dsnopek
reviewed
Jan 29, 2025
| "$<$<STREQUAL:${GODOT_PRECISION},double>:.double>" | ||
| "$<1:.${SYSTEM_ARCH}>" | ||
| # missing IOS_SIMULATOR | ||
| # missing THREADS |
Collaborator
There was a problem hiding this comment.
The threads vs no threads distinction a pretty important one. A reusable GDExtension (ie not one for a specific project) that supports web will very likely want to have builds for both
Collaborator
Author
There was a problem hiding this comment.
Given that its important, I have submitted #1698 to make that happen.
previous version created a list, which is semicolon delimited and then string REPLACED the semicolon to create a single string. New version uses string APPEND to create the string the first time.
- Using rpath for a static library is irrelevant, needs to be seet in consumers. - Add INTERFACE_POSITION_INDEPENDENT_CODE to propogate flag to consumers.
- Add TODO for missing options - Fix formatting - Set GODOT_ARCH in case of arch being 'universal' - Simplify the test project when SYSTEM_NAME is Darwin
- remove incorrectly propagated target_compile_features - remove GNU_ prefix on generic compiler version genex's - remove line breaks for simple genex's - make most flags private - comments
- Add TODO items for missing options - Add missing toolchain managed options commented out - Homogenise Headings and block comments
- This replicates how SCons does it. - Modify test project to use GODOT_SUFFIX - Add GODOT_SUFFIX to libgodot-cpp properties
Collaborator
Author
|
I'm going to split this apart and resubmit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While working on actual features I ran into a fair few small refactors I could make to improve the quality of the solution that didnt necessarily square with the subject of said features.
This PR has each of these under a different commit and will require squashing before merging.
I don't know if this is an acceptable style of change though, so I'm making a draft PR to get feedback.
I can pull out any of these into their own PR or re-arrange as desired.
List of changes:
SYSTEM_NAME
Was set using a list and then using regex replace on the ';' to transform it into a long string.
This was completely unnecessary as
string( APPENDdoes the equivalent thing in one commandUpdate Target Properties
Update macos.cmake and test target
Refactor common_compiler_flags.cmake
Update Comments
replace ad-hock name construction with new GODOT_SUFFIX
web suffix