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

Drop JuceHeader.h, include JUCE modules directly #4816

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

mvf
Copy link
Collaborator

@mvf mvf commented Aug 9, 2021

JuceHeader.h is a generated header that includes all enabled JUCE
modules. This was bloating the preprocessed sources of many Surge TUs by
hundreds of KBytes each (on Linux), slowing down the build somewhat.

Also, JuceHeader.h is generated at build time, impairing IDE navigation
and completion until halfway through an initial build.

This change includes only the required JUCE modules directly, which
is supported in CMake projects as per the documentation.

@mvf mvf requested a review from baconpaul August 9, 2021 15:47
@mkruselj
Copy link
Collaborator

mkruselj commented Aug 9, 2021

Lovely!

JuceHeader.h is a generated header that includes all enabled JUCE
modules. This was bloating the preprocessed sources of many Surge TUs by
hundreds of KBytes each (on Linux), slowing down the build somewhat.

Also, JuceHeader.h is generated at build time, impairing IDE navigation
and completion until halfway through an initial build.

This change includes only the required JUCE modules directly, which
is supported in CMake projects as per the documentation.
@mvf mvf force-pushed the pr/juceheader-bgone branch from b4d3886 to cec4b9b Compare August 9, 2021 17:07
@mvf
Copy link
Collaborator Author

mvf commented Aug 9, 2021

Thanks, the build time savings are fairly tiny though. My Linux debug build went from ~2m45s to ~2m37s. Of course it's all in Surge code, which gets built incrementally all the time, and it might be more noticeable on other platforms. For me the main benefit is CLion not being dazed and confused in fresh projects because of the missing header. JuceHeader.h also masked the fact that we were not applying our OS_COMPILE_DEFINITIONS outside of surge-shared.

Hm, and clang-format 12 wants to make curly brace related changes all over the place. Seems some default has changed since version 10 that's running in the pipelines. Gotta look into that...

@baconpaul
Copy link
Collaborator

Cool I will try and look tonight
I almost never use go to header in clion and instead use go to symbol which is why I wasn’t bothered much but a good change indeed

@mvf
Copy link
Collaborator Author

mvf commented Aug 9, 2021

I almost never use go to header in clion and instead use go to symbol

Same here, and that "go to" function − Ctrl+Click or gd − wasn't working for JUCE types until that header was built, e.g. things would look like this after loading a fresh project:

clion
It's not a huge deal, just a bit annoying for CMake plumbers who build clean all the time. 😅

@baconpaul baconpaul merged commit b0bb41c into surge-synthesizer:main Aug 10, 2021
@mvf mvf deleted the pr/juceheader-bgone branch August 10, 2021 05:53
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.

3 participants