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

[3.x] SCons: Refactor LTO options with lto=<none|thin|full> #63309

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

akien-mga
Copy link
Member

Adds support for LTO on macOS and Android.
Disable LTO by default on iOS even if production=yes is set.

Also add linker option to server platform missed in #63283.

Refactor code handling old arguments to make it simpler (breaks compat,
but is explicit enough about it and scripts are easy to fix).

3.x backport of #63288.

@akien-mga akien-mga added this to the 3.5 milestone Jul 22, 2022
@akien-mga akien-mga requested review from a team as code owners July 22, 2022 10:15
@akien-mga akien-mga requested a review from a team as a code owner July 22, 2022 10:48
@akien-mga akien-mga marked this pull request as draft July 22, 2022 11:43
@@ -100,6 +102,27 @@ def configure(env):
env.extra_suffix = ".llvm" + env.extra_suffix
env.Append(LIBS=["atomic"])

# Linker
if env["linker"] != "default":
Copy link

Choose a reason for hiding this comment

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

i think you need to separate this part to few functions that will handle each case, currently its way to complicated for one functions, stick to SRP and try to make each function do only one thing, also think about OCP, if we want to append this part? how difficult it will be? if a module will change, do we need to rewrite the whole function or you can create a mini function that will do this part? also, use consts for constant strings so if it will be changed in the future you can just replace it in the beginning instead of rewriting that in every part it appears.

env.Append(CCFLAGS=["-flto=thin"])
env.Append(LINKFLAGS=["-flto=thin"])
elif not env["use_llvm"] and env.GetOption("num_jobs") > 1:
env.Append(CCFLAGS=["-flto"])
Copy link

Choose a reason for hiding this comment

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

same here about OCP and SRP, you can even make a diction that will contain the mini functions as a reference and it will make the code much easier to understand

Adds support for LTO on macOS and Android.
Disable LTO by default on iOS even if `production=yes` is set.

Also add `linker` option to `server` platform missed in godotengine#63283.

Refactor code handling old arguments to make it simpler (breaks compat,
but is explicit enough about it and scripts are easy to fix).
@akien-mga akien-mga marked this pull request as ready for review September 5, 2022 12:45
@akien-mga akien-mga merged commit 953dea1 into godotengine:3.x Sep 8, 2022
@akien-mga akien-mga deleted the 3.x-scons-refactor-lto branch September 8, 2022 09:24
@YuriSizov YuriSizov modified the milestones: 3.x, 3.6 Dec 2, 2023
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