FR: Add explicit includes to JUCE modules

[eyalamir]

@anthony-nicholls thank you so much for your thoughtful comments, And I’m happy to see you’re working on this and that we’re generally in agreement!

I just wanted to comment on a couple of things:

@anthony-nicholls

Admittedly a lot of this was already done for juce_gui_basics relatively recently so despite being large and relatively complicated I still think this is probably the best module to test against. Most notably because CLion keeps getting used as an argument for this work and I want to actually see it make a difference in that IDE, I think the size of the module still seems like it could be an issue for CLion.

I totally agree that splitting juce_gui_basics is a good idea for a variety of reasons, and I specifically have an FR for that:

FR: more compact JUCE module headers

We have quite a lot (30+) custom JUCE modules used in multiple products, unit tests, etc, and I noticed that adding any JUCE module include to a module can really inflate compile times, compared to the same module without any JUCE includes. It would be great if some of these high level modules could be split into more compact modules. For example: I’m fine with juce_gui_basics bringing in a bunch of widgets, and no backwards compatibility should be broken for that module. But - in modules wit…

I will say that I’d make this a separate task. First split the module, then do the include stuff. I think even though the two have some correlation they’re not really related.

@anthony-nicholls

I can already browse small modules such as juce_osc just fine in CLion out of the box without any changes

CLion is not a good candidate for checking this, as even though it’s my personal favorite IDE, it’s also a very smart IDE that knows how to parse the build system until it completes the code model.

The problem scope would be much better tested/defined in VSCode and Visual Studio.

@anthony-nicholls

Anyway the point that I was trying to make is these changes could impact our ability to refactor later.

I really don’t see how that’s different from the situation today. You need to make juce_gui_basics.cpp compile and includes for juce_gui_basics.h work.

People including internal headers can happen today too. I’m not sure why adding includes would change that, it doesn’t present any API change.

I also want to stress out that having headers or cpp files including other headers is how every codebase in the world works. I don’t see why JUCE has more refactoring burden because they add includes to files.

I kinda feel like this an argument just for argument’s sake and not a real issue that ever presented itself. In real life, if following a refactor juce_Component.h now needs to include juce_String.h in order to be correct this is a part of the refactoring task anyway, doesn’t matter which file you had to add the include to.

Removing a file/function from a public header or just changing the file structure of public API files would indeed lead to breakage but that again has nothing to do with adding includes…

I’m having a real hard time imagining a scenario where #include your code dependencies has ever caused problem in refactoring.

@anthony-nicholls

I don’t agree that no refactoring is required, some internal files will need to be split into header and source files and these will be best placed in a detail namespace (as I recently done for juce_gui_basics) - some cpp files unfortunately currently rely on declarations in other cpp files

I agree, that one is indeed the ‘annoying’ part of the work that’s required. I just meant that high level refactoring like splitting juce_gui_basics into smaller modules or creating a new JUCE build system is not required.

@anthony-nicholls

I’m undecided on just how important I think this is right now

By no means, I’m not saying it’s something urgent that you have to stop what you’re doing for. But I’m also using CLion where things generally works. If you’ll talk to people using VS Code you’ll get different answers on this topic.

@anthony-nicholls

I don’t agree that we shouldn’t add tests for this if we’re accepting it is a “feature” of JUCE.

Have you ever found a popular codebase that tests for this? My guess is not.
Most codebases I’ve seen just add includes, test that the code compiles and works using the presented build system, and run unit tests on all the different supported APIs.

Once you will remove the global module include, your ‘unit test’ for this would be that the code will not compile if you have a a severe include problem. A minor include problem may still occur, but it would be rare.

I also think that the challenge of unit testing this, as well as maintaining the unit test with JUCE changes is EXTREMELY complicated and non-trivial.

This is something you’re suggesting that will make it very hard for you to refactor, since every change will need to be tested in two different build systems, and not just one, and for very little gain since the compiler will catch all the big problems anyway.