-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
Conversation
b2aa3b2
to
8062d17
Compare
@@ -100,6 +102,27 @@ def configure(env): | |||
env.extra_suffix = ".llvm" + env.extra_suffix | |||
env.Append(LIBS=["atomic"]) | |||
|
|||
# Linker | |||
if env["linker"] != "default": |
There was a problem hiding this comment.
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"]) |
There was a problem hiding this comment.
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).
8062d17
to
570e6c1
Compare
Adds support for LTO on macOS and Android.
Disable LTO by default on iOS even if
production=yes
is set.Also add
linker
option toserver
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.